Closed bahadzie closed 2 months ago
Hi @bahadzie, rather than removing the files in _snaps/
altogether, you should run the tests interactively on your local machine, and run testthat::snapshot_accept('vaccination')
if you want to validate the change in the recorded snapshot.
More info in the dedicated testthat vignette: https://testthat.r-lib.org/articles/snapshotting.html
Hi @bahadzie and @Bisaloo - just checking whether this PR is still current and if it needs to be reviewed? @Bisaloo are you reviewing or would it help for me to step in?
No, this was just a drive by comment based on the check results. I have not reviewed the code. I'll leave it up to your expertise.
Okay thanks, we have a P3 meeting scheduled later so will discuss next steps for this PR then. Edit add: of course we'll restore the snapshots.
Hi @bahadzie, thanks for adding these changes. Could you:
1. Update the PR description to say what the changes are and ideally tag the issues being fixed; 2. Rebase this branch on `main`; 3. Update your version of {testthat} (v3.2.1 appears latest) as this seems to be causing changes to the test snapshots; you would also need to revert changes to the test snapshots, and then check if any snapshots really need to be updated. In general, I would be very conservative in accepting changes to a test snapshot, and I would always mention why in the PR or in the commit message; 4. Update the code in `epidemic_peak()` to access columns using [one of the two methods mentioned in this SO post](https://stackoverflow.com/a/8096882/7410366) by Matt Dowle. In today's meeting I said I preferred setting variables to `NULL` at the top of the function (Dowle's option 2), but I see that in `new_infections()` I have gone for option 1 instead which uses `get()`.
Once 2 -- 4 are done, we shall get a better idea of whether checks pass - I expect they will.
Hi @bahadzie, thanks for adding these changes. Could you:
1. Update the PR description to say what the changes are and ideally tag the issues being fixed; 2. Rebase this branch on `main`; 3. Update your version of {testthat} (v3.2.1 appears latest) as this seems to be causing changes to the test snapshots; you would also need to revert changes to the test snapshots, and then check if any snapshots really need to be updated. In general, I would be very conservative in accepting changes to a test snapshot, and I would always mention why in the PR or in the commit message; 4. Update the code in `epidemic_peak()` to access columns using [one of the two methods mentioned in this SO post](https://stackoverflow.com/a/8096882/7410366) by Matt Dowle. In today's meeting I said I preferred setting variables to `NULL` at the top of the function (Dowle's option 2), but I see that in `new_infections()` I have gone for option 1 instead which uses `get()`.
Once 2 -- 4 are done, we shall get a better idea of whether checks pass - I expect they will.
Done
Thanks @bahadzie - I think you might have rebased on an older commit; could you rebase on the current head of origin/main
? That would probably resolve the flagged conflict.
Hi @pratikunterwegs I've rebased on main.
It's currently running the Github Actions. I'll keep you posted.
Thanks, will take a look tomorrow, checks are passing.
Just looking at this PR again, I think it might be better to restrict this PR to the epidemic peak function by resetting this PR to before the vaccination rate fn is added, I expect that should be https://github.com/epiverse-trace/epidemics/pull/152/commits/8bee33369235d994219efdddce87d3ac289adb50. I think #202 should resolve the effective vaccination issue documented in #198.
Key commits from this branch copied into #240 and merged into main
.