Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

update test for dplyr 1.0.8 #274

Closed
wants to merge 2 commits into from
Closed

update test for dplyr 1.0.8 #274

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e7e0363
update test for dplyr 1.0.8
romainfrancois Nov 15, 2021
690697e
simpler workaround: use select(), which can rename as well, instead o…
romainfrancois Nov 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tests/testthat/test-1-readBGEN.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ test_that("same variant infos as with QCTOOL", {
test <- snp_attach(snp_readBGEN(bgen_file, tempfile(), list(IDs), ncores = ncores()))

expect_identical(
dplyr::mutate(test$map[-19, 1:6], chromosome = as.integer(chromosome)),
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))
Copy link

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

Copy link
Owner

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.

Copy link

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.

Copy link
Owner

@privefl privefl Nov 15, 2021 •
edited
Loading

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..

Copy link
Author

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.

Copy link
Owner

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}?

Copy link

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().

Copy link
Owner

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.

Expand Down