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

Hot fix validate pmf #191

Merged
merged 3 commits into from
Sep 28, 2024
Merged

Hot fix validate pmf #191

merged 3 commits into from
Sep 28, 2024

Conversation

kaitejohnson
Copy link
Collaborator

This PR does two things, one which is necessary another which may be up for discussion but does appear to be necessary if we are passing in long pmfs through a config...

  1. Fix the validate_pmf() function. all.equal() needs to be wrapped in isTRUE() because it returns either TRUE or a character with difference (alternatively we could use that error message here, but I like our custom one). https://www.rdocumentation.org/packages/base/versions/3.6.2/topics/all.equal
  2. Adds a test so that we catch something like this next time

The up for debate thing is that:

  1. Adjusting the tolerance. I loaded from the package data and passed to the config and I think passing things around leads to rounding error, so the long infection to hospital admissions delay has a difference of just above 1e-7. I think we should be a bit more flexible with this given we are setting this up for people to pass in delays via a config.

Open to other suggestions e.g. for the pipeline we could one off load the rda from the wwinference package, use the package default, or compute directly but in general I think we want the wwinference package to allow users to specify via a config a long delay so tolerance can be a bit higher IMO.

Copy link

github-actions bot commented Sep 27, 2024

Thank you for your contribution, @kaitejohnson 🚀! Your page is ready to preview here

dylanhmorris
dylanhmorris previously approved these changes Sep 27, 2024
Copy link
Collaborator

@dylanhmorris dylanhmorris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Maybe make the tolerance configurable?

Copy link
Collaborator

@dylanhmorris dylanhmorris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dylanhmorris dylanhmorris merged commit 46e896b into main Sep 28, 2024
6 checks passed
@dylanhmorris dylanhmorris deleted the hot-fix-validate-pmf branch September 28, 2024 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants