Skip to content

Ndefries/epidatasets migration #382

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

Merged
merged 37 commits into from
Oct 28, 2024
Merged

Conversation

nmdefries
Copy link
Contributor

@nmdefries nmdefries commented Sep 19, 2024

Checklist

Please:

  • Make sure this PR is against "dev", not "main".
  • Request a review from one of the current epipredict main reviewers:
    dajmcdon.
  • Make sure to bump the version number in DESCRIPTION and NEWS.md.
    Always increment the patch version number (the third number), unless you are
    making a release PR from dev to main, in which case increment the minor
    version number (the second number).
  • Describe changes made in NEWS.md, making sure breaking changes
    (backwards-incompatible changes to the documented interface) are noted.
    Collect the changes under the next release number (e.g. if you are on
    0.7.2, then write your changes under the 0.8 heading).
  • Consider pinning the epiprocess version in the DESCRIPTION file if
    • You anticipate breaking changes in epiprocess soon
    • You want to co-develop features in epipredict and epiprocess

Change explanations for reviewer

Moves included datasets to the epidatasets package. Some datasets are being used by epiprocess as well, so dataset names change. The data are reexported in epipredict for easy access. Data contents mostly remain the same. The exception is that the Canadian graduate salary dataset now has values for 2017.

@nmdefries nmdefries force-pushed the ndefries/epidatasets-migration branch from 4815e37 to b50d6df Compare September 30, 2024 17:15
@nmdefries nmdefries marked this pull request as ready for review October 15, 2024 20:03
@nmdefries nmdefries requested a review from dajmcdon as a code owner October 15, 2024 20:03
@nmdefries
Copy link
Contributor Author

@dajmcdon Any suggestions on the tests failures? There are also a bunch of warnings about adding new snapshots. The tests pass locally for me.

@dajmcdon
Copy link
Contributor

How are you running them locally? You may have to push changes to those files.

@nmdefries
Copy link
Contributor Author

nmdefries commented Oct 15, 2024

I'm doing rcmdcheck::rcmdcheck() in Rstudio. It doesn't seem to be making new snapshots...

@dajmcdon
Copy link
Contributor

Can you try devtools::test()? It should hopefully produce the same snapshot mistakes. Otherwise, perhaps @dsweber2 can help.

@nmdefries
Copy link
Contributor Author

devtools::test() output is a much closer match, thanks!

@brookslogan
Copy link
Contributor

brookslogan commented Oct 25, 2024

This makes document() run very slowly on my system. profvis with default interval crashes my computer; with interval = 1 gives a bunch of time in warn_missing_s3_exports though I don't think I saw any emitted warnings. (Though I felt see some sort of complaints when documenting before it started taking 8 minutes.)
2024-10-25-144248_1875x218_scrot

@nmdefries
Copy link
Contributor Author

@brookslogan Documenting runs faster for me now. Is it working better for you?

@nmdefries nmdefries requested a review from dajmcdon October 28, 2024 21:15
@nmdefries nmdefries merged commit 7cc4a8f into dev Oct 28, 2024
2 checks passed
@nmdefries nmdefries deleted the ndefries/epidatasets-migration branch October 28, 2024 21:37
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.

4 participants