Add input validation #476

jamesmbaazam · 2023-10-12T14:55:54Z

This PR:

Adds input checking to 5 core functions: estimate_infections(), estimate_secondary(), estimate_truncation(), simulate_infections(), and epinow().
A function check_reports_valid() is added to validate the reports dataset passed to the functions. Tests are added to check its functionality.
The various *_opts() functions are assigned subclasses of the same name as the functions. These are tested against the assigned arguments to ensure the right *_opts() is passed to the right argument. For example, the obs argument in estimate_secondary() is expected to only receive arguments passed through obs_opts() and will error otherwise.

This PR closes #337.

jamesmbaazam · 2023-10-12T15:17:39Z

@seabbs @sbfnk I've implemented a check for the input data to the various estimate_*() functions. It expects the non-date columns to be integer. Tests are currently failing locally because example_data.rds has a "numeric" confirm column. I would prefer to make that conversion at the source but can't find where the data is created. Could you point me to it?

sbfnk · 2023-10-13T11:11:03Z

I'm not sure we want this to be strictly integer as the method should work in principle with e.g. proportions.

The check to me looks good, only think missing I think is a corresponding test.

In the rest of the package we have mostly (although I don't think strictly) taken the approach of importing functions with @importFrom and then not qualifying them with e.g. checkmate:: in the code except when it might be ambiuous.

jamesmbaazam · 2023-10-13T14:16:38Z

I'm not sure we want this to be strictly integer as the method should work in principle with e.g. proportions.

Alright. I'll change it to check for numerics instead of integer.

The check to me looks good, only think missing I think is a corresponding test.

Thanks. It's still a work in progress, hence the PR is a draft.

In the rest of the package we have mostly (although I don't think strictly) taken the approach of importing functions with @importFrom and then not qualifying them with e.g. checkmate:: in the code except when it might be ambiguous.

Noted. I'll make the change.

sbfnk · 2023-10-30T13:33:25Z

Noting that the test coverage PR was cancelled after 6 hours (didn't fail) - might be worth re-running to see if it's a transient GHA issue.

sbfnk

Great work!

Please add yourself as an author to any function where you've made substantial additions.

R/checks.R

R/estimate_infections.R

R/estimate_secondary.R

seabbs · 2023-10-31T17:41:01Z

Please add yourself as an author to any function where you've made substantial additions.

If this is EpiNow2 dev policy we should update the contributing guide.

jamesmbaazam · 2023-10-31T19:27:59Z

I'm not sure this test-coverage failure is related to the PR content.

jamesmbaazam · 2023-10-31T19:33:45Z

Please add yourself as an author to any function where you've made substantial additions.
If this is EpiNow2 dev policy we should update the contributing guide.

Sure. We can have a section on giving credit. We recently did similar for Epiverse here. I'm actually not sure what constitutes a substantial addition. I guess I would define it as a change without which the function would not do as promised? I would imagine that I would qualify as a co-author of these functions when I work on the S3 class structure for each of them. In that case, the return value of the function is the class with certain attributes, which I implemented, so I can consider myself a co-author.

seabbs

This all looks really nice. Looking forward to having this merged. Just a few comments about line breaks (exciting times) and we need a news update for this.

R/checks.R

R/estimate_secondary.R

R/opts.R

sbfnk · 2023-11-01T10:47:22Z

I'm not sure this test-coverage failure is related to the PR content.

I think it's related but I'm not sure how - the corresponding action hasn't been failing on other PRs, e.g.
#467

That said I could not reproduce the timeouts running covr::codecov locally and tests are not timing out within the R CMD check so I'm not sure what's going on.

jamesmbaazam · 2023-11-01T21:14:53Z

That said I could not reproduce the timeouts running covr::codecov locally and tests are not timing out within the R CMD check so I'm not sure what's going on.

Same. It passes locally but with a warning and 3 notes.

❯ checking top-level files ... WARNING
  A complete check needs the 'checkbashisms' script.
  See section ‘Configure and cleanup’ in the ‘Writing R Extensions’
  manual.

❯ checking installed package size ... NOTE
    installed size is 12.3Mb
    sub-directories of 1Mb or more:
      doc       1.1Mb
      extdata   2.1Mb
      help      2.1Mb
      libs      6.1Mb

❯ checking for GNU extensions in Makefiles ... NOTE
  GNU make is a SystemRequirements.

❯ checking files in ‘vignettes’ ... NOTE
  The following directory looks like a leftover from 'knitr':
    ‘figure’
  Please remove from your package.

0 errors ✔ | 1 warning ✖ | 3 notes ✖

sbfnk · 2023-11-03T13:21:03Z

Could you try temporarily removing test-checks.r and see if that fixes it?

sbfnk · 2023-11-03T18:48:46Z

Could you try temporarily removing test-checks.r and see if that fixes it?

Turns out this didn't do the trick. Next steps could be:

roll back more changes (can always reset to 157b5c4 and force push to revert to original state later) if that's even possible (and I realise it might not be)
~~address Update outdated actions #492 (complete stab in the dark but who knows)~~ not an issue as the everything is up to date in the test-coverage action
ignore (but then we lose the codecov action which would be a bit of a shame)

jamesmbaazam · 2023-11-06T16:00:52Z

I'll create a backup branch and try the fixes there. Whichever works will then be applied here.

seabbs · 2023-11-07T16:48:23Z

Could this be a problem with the cache for this action? I just had this issue in epinowcast (but for another action) and resolved it by removing the related cache using gh cache

jamesmbaazam · 2023-11-07T22:55:57Z

I've removed them and re-run the action. Let's see if it fixes it. 🤞🏾

sbfnk · 2023-11-09T09:56:53Z

Maybe temporarily changing the action here to run covr::codecov with quiet = FALSE will help identify the issue?

jamesmbaazam · 2023-11-13T08:23:59Z

This test-coverage failure issue has persisted after several attempts so I'm going to split this PR into smaller parts and see if that helps:

One to add the function that checks the reports argument.
Separate PR's with input checks for each functions.

sbfnk · 2023-11-13T08:44:06Z

I've been able to reproduce the issue using https://github.com/nektos/act (which is a great little tool for investigating GHA issues locally).

Just looking at some runs I started before the weekend it looks like all the tests in test-estimate_infections.R were slowed down to snail's pace, apparently because of an issue in the interaction between codecov and stan_opts objects being passed to do.call() calls. Changing the code so that only pure lists are passed to do.call has resolved the issue. This is implemented in db0bfce which is based off the last commit of the original PR before we started investigating this issue.

To apply this fix in this PR you can:

git reset --hard to 5410759
git merge the fix-codecov branch
force push

jamesmbaazam · 2023-11-13T09:29:23Z

Amazing! Thanks for taking a look @sbfnk. The stan_opts change is my fault. I made that change in ba00fc1.

sbfnk · 2023-11-13T09:33:08Z

It's still a really strange issue that didn't occur when either of us ran codecov::covr. Fingers crossed this fixes it.🤞

jamesmbaazam · 2023-11-13T11:25:14Z

🥳

jamesmbaazam · 2023-11-13T22:26:06Z

This should be ready for merge now.

sbfnk

Amazing stuff! Nearly there.

R/estimate_infections.R

jamesmbaazam force-pushed the input-val branch from ffc0b28 to 0267355 Compare October 12, 2023 16:02

jamesmbaazam force-pushed the input-val branch 2 times, most recently from 102d8ee to 4fe82c7 Compare October 26, 2023 15:16

jamesmbaazam marked this pull request as ready for review October 27, 2023 11:45

jamesmbaazam self-assigned this Oct 27, 2023

jamesmbaazam requested review from seabbs and sbfnk October 27, 2023 16:18

sbfnk requested changes Oct 30, 2023

View reviewed changes

This was referenced Oct 30, 2023

Move secondary_opts() to opts.R for findability #493

Closed

Fix default value for generation_time in regional_epinow() #494

Closed

jamesmbaazam mentioned this pull request Oct 31, 2023

Add section on acknowledging contributions to CONTRIBUTION.md #496

Closed

seabbs requested changes Nov 1, 2023

View reviewed changes

R/checks.R Outdated Show resolved Hide resolved

R/checks.R Outdated Show resolved Hide resolved

R/estimate_secondary.R Outdated Show resolved Hide resolved

R/opts.R Show resolved Hide resolved

jamesmbaazam mentioned this pull request Nov 1, 2023

Safeguards against partial matching #499

Closed

jamesmbaazam force-pushed the input-val branch from 63a2d59 to 157b5c4 Compare November 1, 2023 21:12

jamesmbaazam force-pushed the input-val branch from 0fecd22 to 4cb8892 Compare November 7, 2023 12:34

jamesmbaazam added 7 commits November 10, 2023 12:22

Allow model argument to be null.

20e4f25

Improve typesetting

e06d1f4

Styling

61df073

Fix a wrong function link in docs

7a61847

Remove awkward line breaks

bb89302

Use rlang arg_match() to prevent partial match

46fcf69

Add news item

5410759

jamesmbaazam force-pushed the input-val branch from d1fa59d to 29b0f5b Compare November 10, 2023 12:24

Pass arguments to stan_opts as list in tests

db0bfce

sbfnk mentioned this pull request Nov 13, 2023

Documentation of reported_cases should not refer to integers #506

Closed

jamesmbaazam force-pushed the input-val branch from 29b0f5b to db0bfce Compare November 13, 2023 09:29

jamesmbaazam added 3 commits November 13, 2023 20:58

Linting: Use == to match length-1 scalars, not %in%

9b94515

Linting: Switch logic of ifelse statement

7ccbc70

Linting: contruct paths with file.path() instead of paste0.

3931cbc

jamesmbaazam requested review from sbfnk and seabbs November 13, 2023 22:25

sbfnk requested changes Nov 14, 2023

View reviewed changes

R/estimate_infections.R Outdated Show resolved Hide resolved

jamesmbaazam added 2 commits November 14, 2023 10:35

Linting: Fix indentation

841d340

Fix wrong return value

da6a20d

jamesmbaazam requested a review from sbfnk November 14, 2023 12:57

sbfnk approved these changes Nov 14, 2023

View reviewed changes

sbfnk merged commit 65e6c5a into main Nov 14, 2023
9 checks passed

sbfnk deleted the input-val branch November 14, 2023 13:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add input validation #476

Add input validation #476

jamesmbaazam commented Oct 12, 2023 •

edited

Loading

jamesmbaazam commented Oct 12, 2023 •

edited

Loading

sbfnk commented Oct 13, 2023

jamesmbaazam commented Oct 13, 2023 •

edited

Loading

sbfnk commented Oct 30, 2023

sbfnk left a comment

seabbs commented Oct 31, 2023

jamesmbaazam commented Oct 31, 2023

jamesmbaazam commented Oct 31, 2023

seabbs left a comment

sbfnk commented Nov 1, 2023 •

edited

Loading

jamesmbaazam commented Nov 1, 2023

sbfnk commented Nov 3, 2023

sbfnk commented Nov 3, 2023 •

edited

Loading

jamesmbaazam commented Nov 6, 2023

seabbs commented Nov 7, 2023

jamesmbaazam commented Nov 7, 2023

sbfnk commented Nov 9, 2023

jamesmbaazam commented Nov 13, 2023 •

edited

Loading

sbfnk commented Nov 13, 2023 •

edited

Loading

jamesmbaazam commented Nov 13, 2023

sbfnk commented Nov 13, 2023

jamesmbaazam commented Nov 13, 2023

jamesmbaazam commented Nov 13, 2023

sbfnk left a comment

Add input validation #476

Add input validation #476

Conversation

jamesmbaazam commented Oct 12, 2023 • edited Loading

jamesmbaazam commented Oct 12, 2023 • edited Loading

sbfnk commented Oct 13, 2023

jamesmbaazam commented Oct 13, 2023 • edited Loading

sbfnk commented Oct 30, 2023

sbfnk left a comment

Choose a reason for hiding this comment

seabbs commented Oct 31, 2023

jamesmbaazam commented Oct 31, 2023

jamesmbaazam commented Oct 31, 2023

seabbs left a comment

Choose a reason for hiding this comment

sbfnk commented Nov 1, 2023 • edited Loading

jamesmbaazam commented Nov 1, 2023

sbfnk commented Nov 3, 2023

sbfnk commented Nov 3, 2023 • edited Loading

jamesmbaazam commented Nov 6, 2023

seabbs commented Nov 7, 2023

jamesmbaazam commented Nov 7, 2023

sbfnk commented Nov 9, 2023

jamesmbaazam commented Nov 13, 2023 • edited Loading

sbfnk commented Nov 13, 2023 • edited Loading

jamesmbaazam commented Nov 13, 2023

sbfnk commented Nov 13, 2023

jamesmbaazam commented Nov 13, 2023

jamesmbaazam commented Nov 13, 2023

sbfnk left a comment

Choose a reason for hiding this comment

jamesmbaazam commented Oct 12, 2023 •

edited

Loading

jamesmbaazam commented Oct 12, 2023 •

edited

Loading

jamesmbaazam commented Oct 13, 2023 •

edited

Loading

sbfnk commented Nov 1, 2023 •

edited

Loading

sbfnk commented Nov 3, 2023 •

edited

Loading

jamesmbaazam commented Nov 13, 2023 •

edited

Loading

sbfnk commented Nov 13, 2023 •

edited

Loading