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

Update {epiparameter} usage #100

Merged
merged 36 commits into from
Mar 6, 2024
Merged

Update {epiparameter} usage #100

merged 36 commits into from
Mar 6, 2024

Conversation

CarmenTamayo
Copy link
Contributor

@CarmenTamayo CarmenTamayo commented Nov 28, 2023

Changes in epiparameter usage to update template contents to the latest version of the package:

  • Changed pathogen to disease on in the params section (both label and description) of the .Rmd to avoid confusion, since epidist uses diseases rather than pathogens
  • Changed epidist() to epidist_db() to extract from database (lines 380-385)
  • Changed epidist$params to as.list(get_parameters(si_epiparameter)) (line 387)<- as.list so it can be used with function on line 406
  • Changed to family(epidist) to extract distribution name (line 388)
  • Changed lines 389 and 390 to extract mean and sd from summary statistics
  • Changed {epiparameter} version on renv
  • Changed {epitrix} to {epiparameter} when converting summary statistics to distribution parameters
  • Changed {distcrete} to {epiparameter} when discretising distribution
  • Removed {epitrix} and {distcrete} as they've now been replaced by {epiparameter} functions
  • Changed param from "use_epiparameter" to "use_epiparameter_database" now that {epiparameter} is used also when this param = FALSE
  • Updated epiparameter usage in code chunks to estimate Rt (EpiNow2, R0)

@Bisaloo
Copy link
Member

Bisaloo commented Nov 29, 2023

Please also update the epiparameter version number for renv:

"epiverse-trace/epiparameter@fd250ce", # nolint

You can find the value to use by running sessioninfo::package_info("epiparameter"), which will tell you which version is installed on your local computer.

Copy link
Member

@Bisaloo Bisaloo left a comment

Choose a reason for hiding this comment

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

Could you also update epiparameter usage in the EpiNow2 child document?

@CarmenTamayo
Copy link
Contributor Author

Could you also update epiparameter usage in the EpiNow2 child document?
yes, that's on my to-do list, to update the usage also on the rmd chunks

Comment on lines 401 to 409
si <- epiparameter::discretise(si_epiparameter)
si_x <- seq(1L, to = si$prob_dist$qf(0.999), by = 1L)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
si <- epiparameter::discretise(si_epiparameter)
si_x <- seq(1L, to = si$prob_dist$qf(0.999), by = 1L)
si <- as.function(si_epiparameter)
si_x <- seq(1L, to = quantile(si_epiparameter, 0.999), by = 1L)

Note however that in the case use_epiparameter_database = FALSE, the object si_epiparameter does not exist.

@CarmenTamayo
Copy link
Contributor Author

@joshwlambert @Bisaloo hi both, do you think we could merge this PR? the only issue I've not been able to resolve is issue #127 , where I'm unsure how {epiparameter} could be used to update what's now being done by epitrix/distcrete
Other than that, this could be merged after being reviewed

@joshwlambert
Copy link
Member

@CarmenTamayo I will review this afternoon and get back to you by EOD.

Copy link
Member

@joshwlambert joshwlambert left a comment

Choose a reason for hiding this comment

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

Thanks for making these updates @CarmenTamayo! Mostly looks really good. I've left a few comments throughout the changes with some comments and questions.

@joshwlambert
Copy link
Member

Given the number of commits to number of files changed I think this PR would be a good candidate for a squash merge.

@Bisaloo
Copy link
Member

Bisaloo commented Feb 21, 2024

Please also add yourself as a package author (role = "aut") in the DESCRIPTION before we merge this @CarmenTamayo 🙏

episoap/DESCRIPTION

Lines 4 to 9 in 0b7406f

Authors@R: c(
person("Hugo", "Gruson", , "hugo.gruson+R@normalesup.org", role = c("aut", "cre", "cph"),
comment = c(ORCID = "0000-0002-4094-1476")),
person("Thibaut", "Jombart", role = "aut"),
person("data.org", role = "fnd")
)

@codecov-commenter
Copy link

codecov-commenter commented Feb 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.07%. Comparing base (e21a7c8) to head (f5dd10b).

❗ Current head f5dd10b differs from pull request most recent head 916e8e3. Consider uploading reports for the commit 916e8e3 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #100   +/-   ##
=======================================
  Coverage   73.07%   73.07%           
=======================================
  Files           4        4           
  Lines          26       26           
=======================================
  Hits           19       19           
  Misses          7        7           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@CarmenTamayo
Copy link
Contributor Author

Please also add yourself as a package author (role = "aut") in the DESCRIPTION before we merge this @CarmenTamayo 🙏

episoap/DESCRIPTION

Lines 4 to 9 in 0b7406f

Authors@R: c(
person("Hugo", "Gruson", , "hugo.gruson+R@normalesup.org", role = c("aut", "cre", "cph"),
comment = c(ORCID = "0000-0002-4094-1476")),
person("Thibaut", "Jombart", role = "aut"),
person("data.org", role = "fnd")
)

Done here 013a44d

DESCRIPTION Outdated Show resolved Hide resolved
@joshwlambert
Copy link
Member

Not sure why the test-reports workflow is failing, looking at the setup-r-dependencies@v2 output from sessionInfo() neither {episoap} or it's dependencies for the templates are being installed, even though they were on previous actions runs when the workflow passed. Could use the setup-renv@2 workflow (https://github.com/r-lib/actions/tree/v2-branch/setup-renv) to ensure the packages required to build the reports are installed with their correct versions. @Bisaloo & @chartgerink let me know if you think this is a suitable fix?

@chartgerink
Copy link
Member

@joshwlambert - are we confident that's the underlying problem, really? If the workflow worked before and has not changed, it feels like something else must be causing this. I just re-ran the workflow and it seems like there is a different problem that requires some debugging to make the action

Quitting from lines 215-230 [unnamed-chunk-2] (skeleton.Rmd)
Error in `value[[3L]]()`:
! Package 'ggplot2' version 3.5.0 cannot be unloaded:
 Error in unloadNamespace(package) : namespace 'ggplot2' is imported by 'episoap' so cannot be unloaded
Backtrace:

To fix the workflow, I would see what changed since the last time the workflow succeeded and find the root cause.

@joshwlambert
Copy link
Member

@chartgerink I think the misunderstanding is because I was looking at the first test report workflow to fail. It seems the error in recent workflow failures is different.

The packages being installed by setup-r-dependencies are the most recent versions, or at least newer than specified in the skeleton.Rmd lock file. The error seems to be that the action cannot unload the newer version. This would presumably be resolved by just installing the versions specified in the lock file in the first place, but I'm unsure how to do this without pulling the lock file into a renv.lock file and using the setup-renv workflow mentioned above.

What's the best approach?

CarmenTamayo and others added 23 commits February 29, 2024 16:21
…bjects when manually providing disease data
…er` functions can be used uniformly in subsequent steps
To avoid loading it (and its dependencies) before renv::use() even got the chance to run, thus creating version conflicts between already loaded packages and requested versions.
It seems for some reason that explicitly namespaced packages are automatically loaded in Rmd documents
@CarmenTamayo CarmenTamayo merged commit d36b447 into main Mar 6, 2024
9 checks passed
@CarmenTamayo CarmenTamayo deleted the epiparameter-update branch March 6, 2024 11:22
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.

None yet

5 participants