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

setnames could break out early if identical(old,new) #3783

Closed
MichaelChirico opened this issue Aug 20, 2019 · 4 comments · Fixed by #3784
Closed

setnames could break out early if identical(old,new) #3783

MichaelChirico opened this issue Aug 20, 2019 · 4 comments · Fixed by #3784
Milestone

Comments

@MichaelChirico
Copy link
Member

Am writing a function that works like

if (length(idx <- grep('^_', names(x)))) setnames(x, idx, gsub('^_', '', names(x)[idx])

It would be cleaner to just write

setnames(x, gsub('^_', '', names(x)))

And (in the !length case) let setnames realize old and new are the same & return early for efficiency.

@MichaelChirico
Copy link
Member Author

Actually it already does this in the missing(new) case

data.table/R/data.table.R

Lines 2447 to 2451 in a8e926a

w = which((nx != old) | (is.na(nx) & !is.na(old)))
} else {
w = which(nx != old)
}
if (!length(w)) return(invisible(x)) # no changes

Will file a fix for !missing(new)

@HughParsonage
Copy link
Member

It would be cleaner to just write

setnames(x, gsub('^_', '', names(x)))

I believe this would work already, no? If the pattern in gsub is not matched the replacement is not attempted.

@MichaelChirico
Copy link
Member Author

@HughParsonage it works but setnames still overwrites the names redundantly

@mattdowle
Copy link
Member

mattdowle commented Aug 30, 2019

This case is a nice one. Have been looking at it and @HughParsonage seems right to me: that case (where the pattern is not matched to any column name) does not overwrite the names redundantly. Even where only a few column names are matched, it only updates those. So if those don't touch key or index names, then the key or index name updates will be saved too. The setnames(DT, new) form seems fine then and no improvement possible.
The PR nicely improves the other form (setnames(DT, old, new)) which wasn't doing any of the redundancy checking or saving.

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 a pull request may close this issue.

4 participants