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

test grouping only before tsibble 1.1.5 #480

Merged
merged 2 commits into from
Jul 9, 2024
Merged

Conversation

dsweber2
Copy link
Contributor

@dsweber2 dsweber2 commented Jul 9, 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

Not bumping the version or adding to NEWS because it only changes tests, and is strictly upstream changes.

I added a skip that only tests for grouping's presence if the version of tsibble is before 1.1.5 specifically.

@dsweber2 dsweber2 requested a review from brookslogan July 9, 2024 18:45
@dsweber2
Copy link
Contributor Author

dsweber2 commented Jul 9, 2024

I also pinned tsibble to 1.1.4 via renv::install("[email protected]"), and the tests passed with no skips for that

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.

Thanks, this looks good for repairing the tests. If you haven't already, can you also open an issue to investigate whether we need to "overoverwrite"/avoid this behavior?

@brookslogan brookslogan merged commit e7b04fc into dev Jul 9, 2024
4 checks passed
@brookslogan brookslogan deleted the testTsibblePatch branch July 9, 2024 22:34
@dsweber2
Copy link
Contributor Author

Unless you have specific instances in mind where we would use this to ungroup, I don't think we need to (in which case, we should be using ungroup anyways). If you'd asked me whether as_tibble destroyed grouping before I ran into this, I would've assumed it wouldn't, and would in fact just be a null-op, like they overwrote it to be.

@brookslogan
Copy link
Contributor

brookslogan commented Jul 30, 2024

I plan to include a fix for this in #477, returning dplyr's expectations, as as_tibble.epi_df appears to be used within some join or summarize operation used within our slides. I'm not sure whether it's possibly grouped at this point or not, but this is showing the whatever S3 magic is going on is not completely insulating dplyr from tsibble overwriting as_tibble.grouped_df, and I don't want to find out where it matters. For what it's worth, it's actually my expectation that as_tibble would output a plain tibble; that seems to follow base R things I'm familiar with, and it's clearly seems to be what dplyr wants since there's an explicit dplyr:::as_tibble.grouped_df doing this. [Aside: I'm thinking that maybe we didn't even need an as_tibble.epi_df at all up to this point.]

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.

2 participants