Skip to content

names<- retains sorting attributes #5849

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.

Sign up for GitHub

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
MichaelChirico merged 3 commits into from
Dec 22, 2023
Merged

names<- retains sorting attributes #5849

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6f4ff22
names<- retains sorting attributes
MichaelChirico Dec 21, 2023
406b3d7
Add tests from #5133
MichaelChirico Dec 21, 2023
bb70dd6
Add colnames<- method, setalloccol() output to enable set()
MichaelChirico Dec 21, 2023
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 R/data.table.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2254,7 +2254,7 @@ dimnames.data.table = function(x) {
{
# When non data.table aware packages change names, we'd like to maintain the key.
# If call is names(DT)[2]="newname", R will call this names<-.data.table function (notice no i) with 'value' already prepared to be same length as ncol
x = shallow(x) # `names<-` should not modify by reference. Related to #1015, #476 and #825. Needed for R v3.1.0+. TO DO: revisit
x = .shallow(x, retain.key=TRUE) # `names<-` should not modify by reference. Related to #1015, #476 and #825. Needed for R v3.1.0+. TO DO: revisit
if (is.null(value))
setattr(x,"names",NULL) # e.g. plyr::melt() calls base::unname()
else
Expand Down
8 changes: 8 additions & 0 deletions inst/tests/tests.Rraw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18111,3 +18111,11 @@ test(2238.9, NA %notin% c(1:5, NA), FALSE)

# shift actionable error on matrix input #5287
test(2239.1, shift(matrix(1:10, ncol = 1)), error="consider wrapping")

# names<- retains index and key, #5125, #5126, #5126, #5128
Copy link
Copy Markdown
Member

@jangorecki jangorecki Dec 21, 2023

Choose a reason for hiding this comment

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

Great work.
Could we add rest of unit tests added in #5133?
If some of them fails then possibly comment them out with explanation. So we have clarity what from #5133 is being covered.

Copy link
Copy Markdown
Member Author

@MichaelChirico MichaelChirico Dec 21, 2023

Choose a reason for hiding this comment

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

good idea. I skipped originally bc I wasn't sure whether output change tests are worth copying.

I'll read more carefully and copy over the worthwhile tests.

Copy link
Copy Markdown
Member Author

@MichaelChirico MichaelChirico Dec 21, 2023

Choose a reason for hiding this comment

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

OK, copied over the tests. They caught two things:

  1. colnames(x) <- is now treated the same a names(x) <-. Requires a new S3 method registration.
  2. Doing names(x) <- "..."; set(x, j=j, val=val) is fixed with a simple setalloccol(). Maybe not ideal, but a good expedient.

Copy link
Copy Markdown
Member

@jangorecki jangorecki Dec 22, 2023

Choose a reason for hiding this comment

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

I don't see any tests being commented out. Does it mean we cover all tests from Matt's PR?

Copy link
Copy Markdown
Member Author

@MichaelChirico MichaelChirico Dec 22, 2023

Choose a reason for hiding this comment

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

I didn't copy over anything where the only diff is in error/message output, i.e.

https://github.com/Rdatatable/data.table/pull/5133/files#diff-e3243f3780ce7d303c3317f73945310bfc37e45d193568246246aca20e3270aeR1336-R1341

https://github.com/Rdatatable/data.table/pull/5133/files#diff-e3243f3780ce7d303c3317f73945310bfc37e45d193568246246aca20e3270aeR1354-R1359

The new tests of actual code are copied over and pass:

https://github.com/Rdatatable/data.table/pull/5133/files#diff-e3243f3780ce7d303c3317f73945310bfc37e45d193568246246aca20e3270aeR1532-R1552

DT <- data.table(a = 1, b = 2)
setkey(DT, a)
setindex(DT, b)
names(DT) <- c("c", "d")
test(2240.1, key(DT), "c")
test(2240.2, indices(DT), "d")