Handle messaging around model treatment of missingness #774

jamesmbaazam · 2024-09-16T10:21:01Z

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

sbfnk

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

NEWS.md

R/opts.R

tests/testthat/test-obs_opts.R

NEWS.md

sbfnk

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)?

R/checks.R

R/estimate_infections.R

R/opts.R

R/estimate_infections.R

jamesmbaazam · 2024-09-20T17:50:45Z

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 · 2024-09-24T08:23:05Z

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.

jamesmbaazam · 2024-10-09T09:09:51Z

Converting this PR to a draft to start a discussion before opening for review.

I'm a little torn and need your opinion (@sbfnk & @seabbs). I have two options here:

Allow check_na_setting_against_data() to call test_data_complete() so that the check is self-contained; small issue is they will share the cols_to_check argument. I think that argument doesn't make sense in the signature of check_na_setting_against_data(). This couples the two functions and there isn't complete separation of concerns.
Only call check_na_setting_against_data() if test_data_complete() returns false

Here, the change required in estimate_infections() and estiamte_secondary() will be:

if (!test_data_complete(data, cols_to_check)) {
		check_na_setting_against_data(data, obs)
}

This second option would mean that check_na_setting_against_data(data, obs) would not check if the data is complete and would unconditionally throw the message about treatment of NA's.

Which one makes sense to you? Is there an option I'm missing?

UPDATE:

I decided to go with option 1.

sbfnk

Looks good to me - just needs the reviewer / PR number added.

sbfnk · 2024-10-11T14:22:53Z

I think the check failures might be related to epinowcast/actions#24 - I'll clear the cache and run again.

jamesmbaazam · 2024-10-14T15:15:11Z

I think the check failures might be related to epinowcast/actions#24 - I'll clear the cache and run again.

It was related to future being required to to run test-setup-future. I now install it in the CI in #827. Will rebase this on main when #827 is merged.

sbfnk · 2024-10-14T15:34:16Z

I think the check failures might be related to epinowcast/actions#24 - I'll clear the cache and run again.

It was related to future being required to to run test-setup-future. I now install it in the CI in #827. Will rebase this on main when #827 is merged.

That was an additional issue - this one was related to the cache (and was successful after clearing it): https://github.com/epiforecasts/EpiNow2/actions/runs/11290308841

Agreed.

Co-authored-by: Sebastian Funk <[email protected]>

jamesmbaazam commented Sep 16, 2024

View reviewed changes

R/opts.R Outdated Show resolved Hide resolved

jamesmbaazam mentioned this pull request Sep 16, 2024

Add vignette on modelling temporally aggregated data (i.e., obs_opts(na = "accumulate") ) #772

Open

sbfnk requested changes Sep 16, 2024

View reviewed changes

jamesmbaazam changed the title ~~Add message about NA treatment in obs_opts()~~ Conditionally throw message for obs_opts(na = "missing") Sep 17, 2024

jamesmbaazam commented Sep 17, 2024

View reviewed changes

tests/testthat/test-obs_opts.R Outdated Show resolved Hide resolved

jamesmbaazam commented Sep 17, 2024

View reviewed changes

NEWS.md Show resolved Hide resolved

jamesmbaazam requested a review from sbfnk September 18, 2024 16:32

sbfnk requested changes Sep 19, 2024

View reviewed changes

jamesmbaazam mentioned this pull request Sep 19, 2024

Distinguish NA (missing) from NA (accumulated) #547

Open

jamesmbaazam force-pushed the na-warning branch from 2ad30a4 to 778b3eb Compare September 20, 2024 17:41

jamesmbaazam force-pushed the na-warning branch from ba50dc2 to 4e4b8b5 Compare September 24, 2024 10:03

jamesmbaazam force-pushed the na-warning branch from e9ec4b5 to 07d5b2d Compare October 8, 2024 15:29

jamesmbaazam marked this pull request as draft October 9, 2024 08:48

jamesmbaazam marked this pull request as ready for review October 10, 2024 12:06

jamesmbaazam requested a review from sbfnk October 10, 2024 12:06

sbfnk previously approved these changes Oct 11, 2024

View reviewed changes

sbfnk enabled auto-merge October 11, 2024 08:18

jamesmbaazam dismissed sbfnk’s stale review via 056da36 October 11, 2024 09:57

This comment was marked as outdated.

Sign in to view

This was referenced Oct 14, 2024

R CMD check failing on main #826

Closed

Install future and future.apply to fix R CMD CHECK #827

Merged

jamesmbaazam force-pushed the na-warning branch from d5f9d9e to 056da36 Compare October 14, 2024 15:12

jamesmbaazam changed the title ~~Conditionally throw message for obs_opts(na = "missing")~~ Handle messaging around model treatment of missingness Oct 15, 2024

jamesmbaazam force-pushed the na-warning branch from 056da36 to d5cadd9 Compare October 15, 2024 15:52

jamesmbaazam and others added 24 commits October 16, 2024 14:27

Add more tests to obs_opts

9f356ac

Remove global variable

225dac6

Use alternative syntax to avoid linting issues

6f9f4e9

copy before modifying data.table

eecf8a1

Fix test

165fbce

Update NEWS.md

9548900

Make message/warning verbose and test for message/warning

77f74af

Simplify comment

84823dc

Use vapply for type safety

9544434

Add lifecycle badge

5b1ce6e

Add lifecycle badge

27069a5

Improve docs

ceab729

Return unmodified obs_opts

94dccd7

Move removal of element of obs_opts() into checker function

aa85bf6

Use setequal instead of checking lengths

f515a94

Remove new unnecessary new lines

4a068fc

Remove trailing white space

6d9f2cf

Add test to check for expected element introduced

4a09c46

Add new argument to pass column names

71fd1eb

Add error for column mismatch

7a0d2c4

Refactor for readability

c405293

Perform missingness check on the raw data

28c373f

Remove test that no longer applies

b1b1efb

Add reviewer and PR number

9c578e5

Co-authored-by: Sebastian Funk <[email protected]>

jamesmbaazam force-pushed the na-warning branch from 96bca23 to 9c578e5 Compare October 16, 2024 13:27

sbfnk added this pull request to the merge queue Oct 16, 2024

Merged via the queue into main with commit f5927c4 Oct 16, 2024
9 checks passed

sbfnk deleted the na-warning branch October 16, 2024 16:15

jamesmbaazam mentioned this pull request Oct 16, 2024

Fix NEWS.md #832

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle messaging around model treatment of missingness #774

Handle messaging around model treatment of missingness #774

jamesmbaazam commented Sep 16, 2024 •

edited

Loading

sbfnk left a comment

sbfnk left a comment

jamesmbaazam commented Sep 20, 2024

sbfnk commented Sep 24, 2024

jamesmbaazam commented Oct 9, 2024 •

edited

Loading

sbfnk left a comment

This comment was marked as outdated.

sbfnk commented Oct 11, 2024

jamesmbaazam commented Oct 14, 2024

sbfnk commented Oct 14, 2024 •

edited by jamesmbaazam

Loading

Handle messaging around model treatment of missingness #774

Handle messaging around model treatment of missingness #774

Conversation

jamesmbaazam commented Sep 16, 2024 • edited Loading

Description

Initial submission checklist

After the initial Pull Request

sbfnk left a comment

Choose a reason for hiding this comment

sbfnk left a comment

Choose a reason for hiding this comment

jamesmbaazam commented Sep 20, 2024

sbfnk commented Sep 24, 2024

jamesmbaazam commented Oct 9, 2024 • edited Loading

sbfnk left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

sbfnk commented Oct 11, 2024

jamesmbaazam commented Oct 14, 2024

sbfnk commented Oct 14, 2024 • edited by jamesmbaazam Loading

jamesmbaazam commented Sep 16, 2024 •

edited

Loading

jamesmbaazam commented Oct 9, 2024 •

edited

Loading

sbfnk commented Oct 14, 2024 •

edited by jamesmbaazam

Loading