names(.SD) should work #4163

ColeMiller1 · 2020-01-08T02:33:02Z

Closes #795. Towards #3189.

This is the proposed implementation:

dt[, names(.SD) := lapply(.SD, '*', 5)]

Update Reference Semantics Vignette
Update .SD Usage Vignette
Update assign help
Include tests

TO DO after release:
Update StackOverflow post to mirror updated .SD vignette.

jangorecki

my vote is for only names(.SD) in LHS and not .SD
even if we decide to use just .SD on LHS then still names(.SD) should works as well

R/data.table.R

inst/tests/tests.Rraw

man/assign.Rd

ColeMiller1 · 2020-01-08T12:07:04Z

Thank you, Jan. I am working on better test cases that do not involve logicals. I will update tonight.

I also noticed that #4031 would be partially addressed:

dt = data.table(iris)
keep = c('Species', 'Sepal.Width')
dt[, .SD := NULL, .SDcols = !keep]

I will try to substitute names(.SD) with the sdvars - that would allow solutions like yours setdiff(names(.SD), keep) work and would provide the option of paste0(names(.SD), 'max') so you can update all of the columns without having to use .SDcols or an intermediate character vector.

R/data.table.R

codecov · 2020-01-14T23:39:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.50%. Comparing base (958e3dd) to head (8fe60ee).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4163   +/-   ##
=======================================
  Coverage   97.50%   97.50%           
=======================================
  Files          80       80           
  Lines       14884    14884           
=======================================
  Hits        14513    14513           
  Misses        371      371

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

ColeMiller1 · 2020-01-14T23:50:25Z

This should be good to go unless we want to support ncol(.SD) or length(.SD) which may be expected to work on the LHS of :=. I could see that being useful for lapply with multiple functions.

Regarding codecov, I am not sure what to do. There are 10 new tests to address 7 lines of new code plus all previous tests using lhs := rhs would also use this code.

jangorecki

Some minor comments.
Support for length(.SD), ncol(.SD) would be nice but that would generally require a different approach, probably an temporary object relating to subset of self. Current approach is fine for most of use cases.
Thing to decide IMO is if we really want to support .SD on LHS which breaks consistency about column names there. My vote goes for an extra names() to be necessary, so we maintain consistency in API.
We also have .SD related vignette so would be good to put some examples there, but that we can always do as a follow-up.

R/data.table.R

man/assign.Rd

ColeMiller1 · 2020-01-20T01:16:03Z

Should be good again. The .SD vignette could be updated with a few items such as patterns() and the ability of using functions. I am happy to update the .SD vignette with this PR, but am unsure since they were done outside of this PR.

But there are two examples that this and Michael's recent PR will work great with:

fkt_idx = which(sapply(Teams, is.factor))
Teams[, (fkt_idx) := lapply(.SD, as.character), .SDcols = fkt_idx]
## to
Teams[, names(.SD) := lapply(.SD, as.character), .SDcols = is.factor]

##From 
team_idx = grep('team', names(Teams), value = TRUE)
Teams[, (team_idx) := lapply(.SD, factor), .SDcols = team_idx]
## To
Teams[, names(.SD) := lapply(.SD, factor), .SDcols = patterns('team')]

I may have went too far. There's no use of ```(cols) := ...``` now but there is at least a reference to the other vignette.

MichaelChirico

IINM we're missing one more test, namely a case where .SD is used as well as some columns not in .SD, e.g.

DT[, names(.SD) := lapply(.SD, \(x) x / b), .SDcols = "a"]

That way we make sure we're picking up the right ".SD" since in that case the "actual" .SD gets augmented with b to make sure it's around during eval.

vignettes/datatable-reference-semantics.Rmd

MichaelChirico · 2024-03-15T01:07:28Z

Friendly ping :) would be great to get this merged!

ColeMiller1 · 2024-03-18T13:26:56Z

I will make changes tonight or by the end of tomorrow night. Thanks for going through everything.

Cole

ColeMiller1 · 2024-03-19T03:00:57Z

I will give the vignette's another look tomorrow and give you a final ping when I'm done.

NEWS.md

MichaelChirico · 2024-03-19T05:12:19Z

R/data.table.R

+              for (i in 2:length(e)) if (!is.null(e[[i]])) e[[i]] = replace_names_sd(e[[i]], cols)
+              e
+            }
+            lhs = eval(replace_names_sd(lhs, sdvars), parent.frame(), parent.frame())


A simpler implementation might be the following:

e <- copyenv(parent.frame()) # pseudocode e$.SD <- setNames(logical(length(sdvars)), sdvars) # or vector("list"), or even vector("raw") to really scrimp on storage lhs = eval(lhs, e, e)

WDYT?

MichaelChirico · 2024-03-19T05:17:27Z

vignettes/datatable-reference-semantics.Rmd

+#### -- How can we update multiple existing columns in place using `.SD`?
+
+```{r}
+flights[, names(.SD) := lapply(.SD, as.factor), .SDcols = is.character]


Something I can imagine will happen soon is a user trying:

x[, names(.SD)[1:5] := ...]

Does that work already? If so, please add a test. If not, no need to handle it until it's requested later unless you see an easy fix.

vignettes/datatable-reference-semantics.Rmd

vignettes/datatable-sd-usage.Rmd

MichaelChirico · 2024-03-19T05:26:04Z

Looks great! Last thing is checking if we can simplify implementation / possibly make it much more general.

ColeMiller1 · 2024-03-20T01:55:33Z

I attempted to make it more general per your suggestion. I copied the implementation below. All the tests pass and it supports something like dt[, names(.SD)[1:5] :=...]. After the commit, I checked to see if base::names(.SD) works and it would as well.

It's easy to revert back or to go down a route you suggested as well. Let me know and I add a base::names(.SD) test, too, depending on how we go.

# i.e lhs is names(.SD) || setdiff(names(.SD), cols) || (cols)

lhs = eval(lhs, list(.SD = setNames(logical(length(sdvars)), sdvars)), parent.frame())

TysonStanley · 2024-03-20T02:02:29Z

I'm a personal fan of the ability to use dt[, names(.SD)[1:5] :=...]. Would be a really nice feature to have that flexibility.

MichaelChirico · 2024-03-20T05:15:51Z

New version looks great, awesome!

R/data.table.R

MichaelChirico · 2024-03-20T05:18:42Z

Almost 10 years later, it's a one-line change 😎

Great stuff!

ColeMiller1 added 9 commits January 7, 2020 19:46

Update data.table.R

316c42b

Update tests.Rraw

e9ae7d3

Update data.table.R

17e80c4

Update tests.Rraw

d1c7a99

Update datatable-reference-semantics.Rmd

9d26aa7

Update assign.Rd

54f35f6

Update NEWS.md

3c68d6e

Update NEWS.md

a009df0

Merge branch 'master' into names_SD

7bd494d

jangorecki reviewed Jan 8, 2020

View reviewed changes

R/data.table.R Outdated Show resolved Hide resolved

inst/tests/tests.Rraw Outdated Show resolved Hide resolved

inst/tests/tests.Rraw Outdated Show resolved Hide resolved

man/assign.Rd Outdated Show resolved Hide resolved

jangorecki added the WIP label Jan 8, 2020

ColeMiller1 added 3 commits January 8, 2020 21:45

Update data.table.R

18ccd2f

Update tests.Rraw

15e95f8

Update tests.Rraw

112f81d

jangorecki reviewed Jan 9, 2020

View reviewed changes

R/data.table.R Outdated Show resolved Hide resolved

ColeMiller1 added 4 commits January 9, 2020 21:26

Update data.table.R

2c39630

Update tests.Rraw

fcb270a

replace iris with raw dataset

21d3a93

Update tests.Rraw

10b36db

jangorecki reviewed Jan 17, 2020

View reviewed changes

R/data.table.R Outdated Show resolved Hide resolved

man/assign.Rd Outdated Show resolved Hide resolved

ColeMiller1 added 4 commits January 19, 2020 08:01

update replace_names_sd and made .SD := not work

7993419

change .SD to names(.SD)

269967e

update typo; change .SD to names(.SD)

76b5e64

update to names(.SD)

ed879f6

ColeMiller1 added 2 commits January 20, 2020 20:40

include names(.SD) and fx to .SD usage

1fbd631

I may have went too far. There's no use of ```(cols) := ...``` now but there is at least a reference to the other vignette.

Updates news to names(.SD)

8df7af5

MichaelChirico reviewed Feb 27, 2024

View reviewed changes

vignettes/datatable-reference-semantics.Rmd Outdated Show resolved Hide resolved

bad merge

be720a3

ColeMiller1 added 2 commits March 18, 2024 22:33

Make updates per Michael's comments.

7b0f8f1

Added test where .SD is used as well as some columns not in .SD.

3635c3d

MichaelChirico reviewed Mar 19, 2024

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

Mention count of reactions in issue

5fec7bc

MichaelChirico reviewed Mar 19, 2024

View reviewed changes

vignettes/datatable-reference-semantics.Rmd Outdated Show resolved Hide resolved

small copy-edit

7ae1ea3

MichaelChirico reviewed Mar 19, 2024

View reviewed changes

vignettes/datatable-sd-usage.Rmd Outdated Show resolved Hide resolved

more specific

2cb48ea

MichaelChirico reviewed Mar 19, 2024

View reviewed changes

vignettes/datatable-sd-usage.Rmd Outdated Show resolved Hide resolved

specify LHS/RHS

5a587e7

Simplify implementation to probe for names(.SD) and new test

212a774

MichaelChirico reviewed Mar 20, 2024

View reviewed changes

R/data.table.R Outdated Show resolved Hide resolved

fine-tune comment

b91dab5

Merge branch 'master' into names_SD

8fe60ee

MichaelChirico approved these changes Mar 20, 2024

View reviewed changes

MichaelChirico merged commit 3eefbca into Rdatatable:master Mar 20, 2024
5 checks passed

tdhock mentioned this pull request Mar 29, 2024

revdep msmtools multiple failures #6033

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

names(.SD) should work #4163

names(.SD) should work #4163

ColeMiller1 commented Jan 8, 2020 •

edited

jangorecki left a comment •

edited

ColeMiller1 commented Jan 8, 2020

codecov bot commented Jan 14, 2020 •

edited

ColeMiller1 commented Jan 14, 2020

jangorecki left a comment

ColeMiller1 commented Jan 20, 2020 •

edited

MichaelChirico left a comment •

edited

MichaelChirico commented Mar 15, 2024

ColeMiller1 commented Mar 18, 2024

ColeMiller1 commented Mar 19, 2024

MichaelChirico Mar 19, 2024

MichaelChirico Mar 19, 2024

MichaelChirico commented Mar 19, 2024

ColeMiller1 commented Mar 20, 2024 •

edited

TysonStanley commented Mar 20, 2024

MichaelChirico commented Mar 20, 2024

MichaelChirico commented Mar 20, 2024

names(.SD) should work #4163

names(.SD) should work #4163

Conversation

ColeMiller1 commented Jan 8, 2020 • edited

jangorecki left a comment • edited

Choose a reason for hiding this comment

ColeMiller1 commented Jan 8, 2020

codecov bot commented Jan 14, 2020 • edited

Codecov Report

ColeMiller1 commented Jan 14, 2020

jangorecki left a comment

Choose a reason for hiding this comment

ColeMiller1 commented Jan 20, 2020 • edited

MichaelChirico left a comment • edited

Choose a reason for hiding this comment

MichaelChirico commented Mar 15, 2024

ColeMiller1 commented Mar 18, 2024

ColeMiller1 commented Mar 19, 2024

MichaelChirico Mar 19, 2024

Choose a reason for hiding this comment

MichaelChirico Mar 19, 2024

Choose a reason for hiding this comment

MichaelChirico commented Mar 19, 2024

ColeMiller1 commented Mar 20, 2024 • edited

TysonStanley commented Mar 20, 2024

MichaelChirico commented Mar 20, 2024

MichaelChirico commented Mar 20, 2024

ColeMiller1 commented Jan 8, 2020 •

edited

jangorecki left a comment •

edited

codecov bot commented Jan 14, 2020 •

edited

ColeMiller1 commented Jan 20, 2020 •

edited

MichaelChirico left a comment •

edited

ColeMiller1 commented Mar 20, 2024 •

edited