-
Notifications
You must be signed in to change notification settings - Fork 44
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
update test for dplyr 1.0.8 #274
Conversation
This pull request fixes the test. |
Hi Romain. |
The test fails because |
tests/testthat/test-1-readBGEN.R
Outdated
dplyr::relocate(dplyr::mutate(test$map[-19, 1:6], chromosome = as.integer(chromosome)), rsid, .before = "chromosome"), | ||
dplyr::as_tibble(dplyr::transmute( | ||
snp_info[-19, ], chromosome, marker.ID = alternate_ids, rsid, | ||
physical.pos = position, allele1 = alleleA, allele2 = alleleB)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better fix might be to replace the transmute()
in the next line with a rename()
followed by a select()
that selects the columns in the order seen here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sorry I still don't get what the error is here..
chromosome
is the first variable, and should remain the first variable after the mutate()
, which has always been the case, and it seems it is designed to remain like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take transmute(df, chromosome, marker.ID = alternate_ids, rsid)
as the example, which is identical to transmute(df, chromosome = chromosome, marker.ID = alternate_ids, rsid = rsid)
.
By accident, that was previously putting the columns in the order they are specified, ignoring whether or not any of them already existed, so you'd get a column order of (chromosome, marker.ID, rsid)
.
This is inconsistent with how mutate()
works, where the invariants are:
- Modified columns that already exist are overwritten in place (their order can't change)
- Completely new columns are added to the end
So the correct resulting order should have been (chromosome, rsid, marker.ID)
(assuming that in the original data frame, chromosome
was before rsid
), and that is now the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow.. Have you asked the community about this?
In transmute()
, where you basically only keep the columns you are interested in (and forget about all the others), you kind of forget about the order as well and just want the order that you specify (at least this is what I want).
Please keep it that way. This would break so much code..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, there's no need for transmute()
and it can be replaced by select()
which can rename as well. , I've updated the pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you still plan to change the behaviour of transmute()
in the next release of {dplyr}?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@privefl Hi, as explained in tidyverse/dplyr#6086, the behavior seems to have been modified to the original behavior (the behavior of the current CRAN release) again in tidyverse/dplyr#6087 to make it consistent with the past behavior of transmute()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Then I can close this.
I am trying to reproduce the issue with the GitHub version of {dplyr}. |
@privefl this is probably because you need to update |
…f using transmute()
We're about to release dplyr 1.0.8 (tidyverse/dplyr#6080) which would cause this failure in one test:
This is because of this fix: tidyverse/dplyr#6035