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

Import datasets and documentation from epidatasets #520

Merged
merged 40 commits into from
Oct 15, 2024

Conversation

nmdefries
Copy link
Contributor

@nmdefries nmdefries commented Aug 30, 2024

Checklist

Please:

  • Make sure this PR is against "dev", not "main" (unless this is a release
    PR).
  • Request a review from one of the current main reviewers:
    brookslogan, nmdefries.
  • Makes sure to bump the version number in DESCRIPTION. 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
    1.7.2, then write your changes under the 1.8 heading).
  • See DEVELOPMENT.md for more information on the development
    process.

Change explanations for reviewer

Reexport datasets from epidatasets, inheriting all associated documentation. Add data attribution -- since license info is included in dataset documentation, this is just adding data contributors to DESCRIPTION.

Supersedes #344

Closes #327

@nmdefries nmdefries force-pushed the ndefries/epidatasets-migration branch 2 times, most recently from 85e240f to acab84e Compare September 5, 2024 21:47
@nmdefries nmdefries force-pushed the ndefries/epidatasets-migration branch from acab84e to ad225b2 Compare September 16, 2024 20:53
@nmdefries nmdefries marked this pull request as ready for review September 16, 2024 20:57
@nmdefries
Copy link
Contributor Author

Failing document is because the epidatasets version on main doesn't export all of the same data objects.

@nmdefries
Copy link
Contributor Author

This also needs to rebuild docs and add doctor-visits attribution to the DESCRIPTION, once we have that from Roni.

@nmdefries nmdefries force-pushed the ndefries/epidatasets-migration branch from 72b8e91 to e04f5a7 Compare October 2, 2024 15:54
@nmdefries nmdefries force-pushed the ndefries/epidatasets-migration branch from fa93d3f to b6a7d58 Compare October 2, 2024 20:34
@nmdefries nmdefries requested a review from brookslogan October 2, 2024 21:22
brookslogan and others added 2 commits October 3, 2024 13:38
Cumulative death data is in tibble format and isn't really the type of data we
expect in many functions. Probably not good to make it too accessible.
Copy link
Contributor

@brookslogan brookslogan left a comment

Choose a reason for hiding this comment

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

Pushed some tweaks; please

  • double-check tweaks

Only blocker to merging:

  • double-checking whether correlation vignette changes were intentional/okay. If they weren't intentional & you want me to check if changes are okay, please ping me / re-request review.

Others comments are ideas for the future; could you refile to issues if you think it makes sense? Not sure what doc overhaul is going to look like.

vignettes/correlation.Rmd Outdated Show resolved Hide resolved
vignettes/epiprocess.Rmd Outdated Show resolved Hide resolved
vignettes/correlation.Rmd Outdated Show resolved Hide resolved
@brookslogan
Copy link
Contributor

brookslogan commented Oct 3, 2024

Wait, there is also

  • Update DESCRIPTION and NEWS.md given PRs that have been merged. Version should be 0.9.{x+1} but changelog additions will go under NEWS.md's 0.10 heading.

@nmdefries
Copy link
Contributor Author

This now depends on cmu-delphi/epidatasets#5

Copy link
Contributor

@brookslogan brookslogan left a comment

Choose a reason for hiding this comment

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

Added a bit more to the NEWS.md entry; merging.

@brookslogan
Copy link
Contributor

brookslogan commented Oct 14, 2024

Trying to fix up CHECKs now.

@brookslogan brookslogan mentioned this pull request Oct 15, 2024
@brookslogan brookslogan merged commit cb468db into dev Oct 15, 2024
3 checks passed
@brookslogan brookslogan deleted the ndefries/epidatasets-migration branch October 15, 2024 12:08
@brookslogan
Copy link
Contributor

We inherit some soft CHECK complaints from cmu-delphi/epidatasets#10 but I'm going to go ahead and merge.

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.

Properly attribute code and data sources
2 participants