Skip to content

440 epiprocess imports #444

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 10 commits into from
Feb 26, 2025
Merged

440 epiprocess imports #444

merged 10 commits into from
Feb 26, 2025

Conversation

dajmcdon
Copy link
Contributor

Checklist

Please:

  • Make sure this PR is against "dev", not "main".
  • 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).

Change explanations for reviewer

@dajmcdon dajmcdon requested a review from brookslogan February 26, 2025 20:46
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.

Changes look good; have a couple of non-blocking musings.

@@ -166,7 +167,7 @@ forecasts %>%
doctor_visits %>%
epix_as_of(doctor_visits$versions_end) %>%
group_by(geo_value) %>%
tidyr::fill(percent_cli),
fill(percent_cli),
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to do what was intended. Maybe some sort of complete was intended beforehand?

waldo::compare(
doctor_visits %>%
      epix_as_of(doctor_visits$versions_end) %>%
      group_by(geo_value) %>%
      fill(percent_cli)
doctor_visits %>%
      epix_as_of(doctor_visits$versions_end) %>%
      `attr<-`("sorted", NULL) %>%
      `attr<-`(".internal.selfref", NULL) %>%
      group_by(geo_value)
)
#> ✔ No differences

It does unset some attrs that shouldn't be there... that's epiprocess#561

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure where this was introduced. Hopefully it gets handled in #431 @dsweber2

@@ -160,6 +162,7 @@ export(extract_argument)
export(extract_frosting)
export(extract_layers)
export(extrapolate_quantiles)
export(filter)
Copy link
Contributor

Choose a reason for hiding this comment

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

musing: this does introduce a conflict with stats::filter, but I guess eventually we should be providing our own convolution utilities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that will happen anytime dplyr is used. This would have happened before anyway since epiprocess was in Depends and it exports filter().

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... I'm not sure why we export filter from epiprocess; shouldn't be necessary just to provide an S3 method.

Copy link
Contributor

@brookslogan brookslogan Feb 27, 2025

Choose a reason for hiding this comment

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

We could make this happen only when user attaches dplyr themselves. I guess if users are using [dplyr filter] a lot [seems likely] though then it might make sense to re-export. It's just that convolution does seem somewhat popular with potential users.

@dajmcdon dajmcdon merged commit a805c2a into dev Feb 26, 2025
2 checks passed
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.

Move epiprocess from Depends: to Imports:
2 participants