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

Add skip_absent to setcolorder #6044

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

sluga
Copy link
Contributor

@sluga sluga commented Apr 2, 2024

Motivation: I sometimes want to ensure a certain order of columns but the set of columns is not fixed, i.e. a given column may or may not be present.

Now, working around that is trivial & so the benefit of this new argument isn't large, but it would be a small quality of life improvement & furthermore setnames already has the skip_absent argument, so it feels appropriate to add it to setcolorder as well.

Copy link
Member

@MichaelChirico MichaelChirico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert whitespace changes first to make the diff clearer

Copy link

codecov bot commented Apr 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.53%. Comparing base (8476b5b) to head (b667257).
Report is 24 commits behind head on master.

❗ Current head b667257 differs from pull request most recent head b135a29. Consider uploading reports for the commit b135a29 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6044      +/-   ##
==========================================
+ Coverage   97.51%   97.53%   +0.01%     
==========================================
  Files          80       80              
  Lines       14980    14922      -58     
==========================================
- Hits        14608    14554      -54     
+ Misses        372      368       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sluga sluga force-pushed the skip-absent-for-setcolorder branch from 4f95ee1 to b667257 Compare April 3, 2024 07:24
@MichaelChirico
Copy link
Member

Hi @sluga, happy to review but I still see a lot of spurious whitespace diff in R/data.table.R, PTAL

@sluga sluga force-pushed the skip-absent-for-setcolorder branch from b667257 to 16ff336 Compare April 9, 2024 21:10
@sluga sluga force-pushed the skip-absent-for-setcolorder branch from 16ff336 to 0abcb02 Compare April 9, 2024 21:13
@sluga
Copy link
Contributor Author

sluga commented Apr 9, 2024

Missed the indent change. Might not be a bad idea to include a .Rproj file with the correct whitespace config in the repo; I assume at least some contributors use RStudio, and I don't think the file would be a problem for those who don't use RStudio.

@MichaelChirico
Copy link
Member

Missed the indent change. Might not be a bad idea to include a .Rproj file with the correct whitespace config in the repo; I assume at least some contributors use RStudio, and I don't think the file would be a problem for those who don't use RStudio.

Thanks for the suggestion. Those files are ignored:

data.table/.gitignore

Lines 23 to 24 in 585ec52

.Rproj.user
data.table.Rproj

As are several related to other IDEs like Emacs.

Better might be putting in the .dev folder, like our recommended .Rprofile:

https://github.com/Rdatatable/data.table/blob/master/.dev/.Rprofile

There users can get a copy when cloning the repo and just copy it to the package directory.

I am using VScode these days so not sure what an Rproj looks like anymore.

R/data.table.R Outdated Show resolved Hide resolved
R/data.table.R Outdated Show resolved Hide resolved
man/setcolorder.Rd Outdated Show resolved Hide resolved
@MichaelChirico
Copy link
Member

LGTM, please add a NEWS bullet

@MichaelChirico
Copy link
Member

Thanks! Let's merge #6068 first since it's basically ready, then would you mind quickly refactoring this PR to use the new functionality?

@MichaelChirico
Copy link
Member

OK, #6068 is merged, would you like to have a go at using the new argument to colnamesInt()?

@sluga sluga force-pushed the skip-absent-for-setcolorder branch from c76d7da to 67979e3 Compare April 17, 2024 18:28
@sluga
Copy link
Contributor Author

sluga commented Apr 17, 2024

@MichaelChirico Hmm, the tests I added for this feature pass after the update, but test 1967.63 fails:

Test 1967.63 produced 0 errors but expected 1
Expected: Item 2 of order (1) is either NA, out of range [1,2], or is duplicated. The new order must be a strict permutation of 1:n
Observed:

README.md Outdated Show resolved Hide resolved
R/data.table.R Outdated Show resolved Hide resolved
R/data.table.R Outdated Show resolved Hide resolved
@jangorecki jangorecki removed their request for review April 26, 2024 08:51
test(498.11, setcolorder(DT, c('d', 'c', 'b', 'a'), skip_absent=TRUE), data.table(c=3, b=2, a=1))
test(498.12, setcolorder(DT, 4:1), error='non-existing column')
test(498.13, setcolorder(DT, 4:1, skip_absent=TRUE), data.table(a=1, b=2, c=3))
test(498.14, setcolorder(DT, c(1, 1, 2, 3), skip_absent=TRUE), error='!=')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test for setcolorder(DT, c('a', 'b', 'd'), skip_absent=TRUE).

The NEWS item currently reads like 'c' will be dropped from the output, which would be bad -- I would expect the above to be equivalent to setcolorder(DT, c("a", "b")).

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.

None yet

3 participants