-
Notifications
You must be signed in to change notification settings - Fork 33
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
Replace get functions with examples for delay distributions #481
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Content-wise this looks good. I think it goes a bit far the other way in that there are now no examples of using dist_spec
. In particular, in the README some of the text doesn't really make any sense and some parts of the code just show rather pointless reallocation.
The example reporting delay also has no uncertainty which alters a fair number of the examples and tests where it was used. Perhaps we could just have two or have it be uncertain and use the new helper function to make it have no uncertainty (which is a very nice addition).
That makes sense. I'm not sure what the right balance is, or if we even want/need these examples. Will revisit starting from where you pointed out it being an issue.
Yes I think if we make it uncertain (which may have been my original intention) the rest of the code makes sense. |
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 1a634ff is merged into main:
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 8a1117a is merged into main:
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 0026e85 is merged into main:
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 7d96a29 is merged into main:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good but it looks like lazy loading isn't working as expected when combining distributions or directly specifiying them as args. See the rendered version of the epinow vignette for an example. It may be that they need to be explicitly allocated? A little confused about why that would be the case though? Perhaps due to not having the dev version of the package installed and rendering directly rather than using devtools?
This is how benchmark results would change (along with a 95% confidence interval in relative change) if fd5acc2 is merged into main:
|
This is ready for re-review. Note touchstone failure is expecting as the corresponding scripts have changed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Small grammar suggestion and flagging some maybe transient fitting issues in the README (I think this should be its own issue if they start around).
Looks much improved. |
Fixes #390.
Also introduces a
fix_dist
function for removing the uncertainty from delay distributions(😲).