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

Call arrange() after group_by() + summarize() #6

Conversation

DavisVaughan
Copy link
Contributor

Hi there, we ran revdeps for dplyr and your package appeared in our checks.

We have updated arrange() to now have an explicit .locale argument that defaults to American English. It previously respected the system locale, but this is more performant and reproducible across R sessions.

You currently rely on the resulting ordering of group_by(df, x) %>% summarize() matching the ordering you'd get back from arrange(df, x). Instead, we suggest explicitly calling arrange() after calling summarize() if you rely on that ordering, as it will no longer match arrange() automatically in the next version of dplyr.

If you install this PR, you will see that one of your tests fails because of this assumption. tidyverse/dplyr#6263

Luckily, the fix here is simple and work with both dev and CRAN dplyr! If you could merge this PR and then go ahead and send a release to CRAN, then we would greatly appreciate it. It will make releasing the next version of dplyr easier for us.

Thanks!

An explicit ordering is expected here. The result needs to match the ordering in `parms.df`, which is arranged by `item_id`. Rather than relying on the ordering we get back from `summarize()`, it is best to have an explicit call to `arrange()`.
@jessekps
Copy link
Member

Great catch, tnx. I certainly did not intend to rely on group_by's ordering. I'll have to check if it occurs anywhere else. I had planned the next cran release for end of next week and will include this fix

@jessekps jessekps merged commit 9e47860 into dexter-psychometrics:master May 11, 2022
@jessekps
Copy link
Member

the fix is currently on its way to cran

@DavisVaughan
Copy link
Contributor Author

Thank you!

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