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

Conditionally throw message for obs_opts(na = "missing") #774

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

jamesmbaazam
Copy link
Contributor

@jamesmbaazam jamesmbaazam commented Sep 16, 2024

Description

This PR closes #771 by adding a message to inform users about how NA's are treated in the default model. It also points to alternatives given the user's use case. This PR also adds missing tests for obs_opts().

Initial submission checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have tested my changes locally (using devtools::test() and devtools::check()).
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required and rebuilt docs if yes (using devtools::document()).
  • I have followed the established coding standards (and checked using lintr::lint_package()).
  • I have added a news item linked to this PR.

After the initial Pull Request

  • I have reviewed Checks for this PR and addressed any issues as far as I am able.

R/opts.R Outdated Show resolved Hide resolved
Copy link
Contributor

@sbfnk sbfnk left a comment

Choose a reason for hiding this comment

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

Nice - some suggestions for (I think) slightly clearer wording. I think we should be more restrictive about when this message is shown (e.g. it should be suppressed when there is no missing data).

R/opts.R Outdated Show resolved Hide resolved
NEWS.md Show resolved Hide resolved
R/opts.R Outdated Show resolved Hide resolved
R/opts.R Outdated Show resolved Hide resolved
R/opts.R Outdated Show resolved Hide resolved
tests/testthat/test-obs_opts.R Show resolved Hide resolved
@jamesmbaazam jamesmbaazam changed the title Add message about NA treatment in obs_opts() Conditionally throw message for obs_opts(na = "missing") Sep 17, 2024
NEWS.md Outdated Show resolved Hide resolved
Copy link
Contributor

@sbfnk sbfnk left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good.

It's adding a fair amount of infrastructure (an additional function + return value in obs_opts() that needs to be removed) just to be able to show that message. Shall we give it an expiry (and use keyword "deprecated" somewhere)?

# (date, confirm, primary, secondary)
columns_to_check <- c(
"date",
intersect(c("confirm", "primary", "secondary"), names(data))
Copy link
Contributor

Choose a reason for hiding this comment

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

Hardcoding these column names will require coming back to this when addressing #505 - perhaps we should just pass this as an argument (where the function asking for the tests specifies what to check)?

R/checks.R Outdated Show resolved Hide resolved
R/checks.R Outdated Show resolved Hide resolved
R/estimate_infections.R Outdated Show resolved Hide resolved
R/opts.R Outdated Show resolved Hide resolved
R/estimate_infections.R Outdated Show resolved Hide resolved
@jamesmbaazam
Copy link
Contributor Author

Thanks, this looks good.

It's adding a fair amount of infrastructure (an additional function + return value in obs_opts() that needs to be removed) just to be able to show that message. Shall we give it an expiry (and use keyword "deprecated" somewhere)?

Was this what you meant? https://github.com/epiforecasts/EpiNow2/pull/774/files#diff-6cd987920443d42098a4963d43b2e0efec9fd16f9198d66705e6d95094e3b87aR185-R194.

@sbfnk
Copy link
Contributor

sbfnk commented Sep 24, 2024

Thanks, this looks good.
It's adding a fair amount of infrastructure (an additional function + return value in obs_opts() that needs to be removed) just to be able to show that message. Shall we give it an expiry (and use keyword "deprecated" somewhere)?

Was this what you meant? https://github.com/epiforecasts/EpiNow2/pull/774/files#diff-6cd987920443d42098a4963d43b2e0efec9fd16f9198d66705e6d95094e3b87aR185-R194.

Yes, I think this will do.

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.

Add message explaining new behaviour when obs_opts(na = "missing")
2 participants