Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove add_mean_sd if possible #428

Closed
athowes opened this issue Nov 13, 2024 · 9 comments
Closed

Remove add_mean_sd if possible #428

athowes opened this issue Nov 13, 2024 · 9 comments
Assignees

Comments

@athowes
Copy link
Collaborator

athowes commented Nov 13, 2024

See #241.

@athowes athowes added medium Nice to have for next release high Required for next release labels Nov 13, 2024
@athowes
Copy link
Collaborator Author

athowes commented Nov 13, 2024

Also predict_delay_dpar?

@athowes athowes removed the medium Nice to have for next release label Nov 13, 2024
@athowes athowes self-assigned this Nov 13, 2024
@athowes
Copy link
Collaborator Author

athowes commented Nov 14, 2024

Instances of using add_mean_sd outside of the definition, tests, and auto generated files:

  • Used in definition of predict_delay_parameters
  • R/make_hexsticker.R
    • secondary_dist <- add_mean_sd(secondary_dist)
  • vignettes/epidist.Rmd
    • secondary_dist <- add_mean_sd(secondary_dist)
  • vignettes/faq.Rmd
    • Notes the purpose of add_mean_sd for adding columns containing the natural scale mean and standard deviation.

@athowes
Copy link
Collaborator Author

athowes commented Nov 14, 2024

Given the amount of things that add_mean_sd and predict_delay_parameters touch I think it might be too annoying to remove them.

@athowes athowes added medium Nice to have for next release and removed high Required for next release labels Nov 15, 2024
@athowes
Copy link
Collaborator Author

athowes commented Nov 15, 2024

F2F @seabbs agreed this was too much work to do before first release so will handle later.

@seabbs
Copy link
Contributor

seabbs commented Nov 19, 2024

The only thing we want to do is decide if we are happy with the name predict_delay_parameters (i.e does it fit with other usage in tidybayes etc/

@athowes
Copy link
Collaborator Author

athowes commented Nov 19, 2024

It could be add_delay_parameter_draws to be more inline with tidybayes

@seabbs seabbs added high Required for next release and removed medium Nice to have for next release labels Nov 19, 2024
@seabbs
Copy link
Contributor

seabbs commented Nov 19, 2024

That could be nice. it would require changing the arguments around to be newdata, fit vs fit newdata to match that pattern. That approach does become a bit clunky if calling without new data is the only caveat.

As this is the current entry point to the package I think we need to decide on this before release

@athowes athowes added public release and removed high Required for next release labels Nov 20, 2024
@athowes
Copy link
Collaborator Author

athowes commented Nov 20, 2024

  • predict is more standard but everything else is in the package hence we support move to tidybayes approach.
  • Also make sure that the outputs are the same as tidybayes
  • Move to using generic approach with summaries generated from samples
  • Find some way to work around the fact that we would be doing a lot of sampling by default if users put newdata = newdata
  • Provide some helper to move to unique combinations of strata (can look in closed PR for example code doing this)

@athowes
Copy link
Collaborator Author

athowes commented Nov 22, 2024

Closed in favour of #471.

@athowes athowes closed this as not planned Won't fix, can't repro, duplicate, stale Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants