Skip to content

Fix grouping issue in test data #48

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 7 commits into from
Jun 16, 2022
Merged

Conversation

ChloeYou
Copy link
Contributor

Closes #45.

Should this also be merged into the dev branch?

@ChloeYou ChloeYou requested a review from brookslogan June 13, 2022 19:31
@ChloeYou ChloeYou requested a review from dajmcdon as a code owner June 13, 2022 19:31
@dshemetov dshemetov changed the base branch from main to frosting June 13, 2022 22:55
@dshemetov
Copy link
Contributor

dshemetov commented Jun 13, 2022

I just switched this to merge into the frosting branch. If we merge #49 in first (where main is merged into frosting), then the long commit log should disappear here.

@brookslogan
Copy link
Contributor

Right, this PR probably shouldn't be the one to merge main into frosting. Taking a look at #49 to see if it can be merged in first.

@brookslogan
Copy link
Contributor

Also, I assume the merge conflicts GitHub is flagging now might automatically be resolved if #49 is merged.

@dshemetov dshemetov changed the base branch from frosting to main June 14, 2022 22:53
@dshemetov dshemetov changed the base branch from main to frosting June 14, 2022 22:53
@ChloeYou ChloeYou requested a review from brookslogan June 15, 2022 18:24
While `dplyr` appears to automagically provide its own `across` inside
`group_by`, we still want to explicitly use `dplyr::` to
- satisfy package checks,
- continue to work if `dplyr` removes this magic (e.g., if the maintainers don't
  like this magic ignoring any user-defined/attached non-`dplyr` `across`
  function), and
- be clear to code readers&editors where the function comes from.
@brookslogan brookslogan self-requested a review June 16, 2022 11:03
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.

Looks good to resolve #45. I have some other questions and comments about how the function is supposed to work, but I will add them to a separate issue.

@brookslogan brookslogan self-requested a review June 16, 2022 11:07
@brookslogan brookslogan merged commit 9dd8c8a into cmu-delphi:frosting Jun 16, 2022
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.

get_test_data() should return an ungrouped epi_df
4 participants