Skip to content

Add support for patterns(cols=user_provided)#6510

Merged
MichaelChirico merged 6 commits intomasterfrom
fix6498
Sep 25, 2024
Merged

Add support for patterns(cols=user_provided)#6510
MichaelChirico merged 6 commits intomasterfrom
fix6498

Conversation

@tdhock
Copy link
Member

@tdhock tdhock commented Sep 20, 2024

Closes #6498

Before this PR:

> melt(data.table(x1=1,x2=2,y1=3,y2=4),measure.vars=patterns("2",cols=c("y1","y2")))
      x1    y1    y2 variable value
   <num> <num> <num>   <fctr> <num>
1:     1     3     4       x2     2

above result is unexpected since variable=x2 which is not present in cols.

After this PR:

> melt(data.table(x1=1,x2=2,y1=3,y2=4),measure.vars=patterns("2",cols=c("y1","y2")))
      x1    x2    y1 variable value
   <num> <num> <num>   <fctr> <num>
1:     1     2     3       y2     4

above result is better since y2 variable is present in cols.

this provides functionality similar to #6077

@tdhock
Copy link
Member Author

tdhock commented Sep 20, 2024

on my local windows machine, all tests pass.
this could be a "breaking change" since patterns now outputs character instead of integer.
But when patterns() is used in the context of melt() or .SDcols, it gives the same result before/after this PR. (so no breakage for the typical use case)
And I don't think people should be using patterns() otherwise (although it is exported).

@hongyuanjia
Copy link
Contributor

Thanks @tdhock. I love this change. For me, and I believe most DT users, the primary use of patterns is to extract columns in melt and .SDcols, so no break changes in this aspect.

@github-actions
Copy link

github-actions bot commented Sep 20, 2024

Comparison Plot

Generated via commit d3bea19

Download link for the artifact containing the test results: ↓ atime-results.zip

Time taken to finish the standard R installation steps: 3 minutes and 13 seconds

Time taken to run atime::atime_pkg on the tests: 9 minutes and 21 seconds

Copy link
Member

@Anirban166 Anirban166 left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

patterns() did not consider the position of provided cols in the original DT

4 participants