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

DT assign by ref not accept list col as RHS, error msg misleading #950

Closed
jangorecki opened this issue Nov 14, 2014 · 11 comments · Fixed by #3457
Closed

DT assign by ref not accept list col as RHS, error msg misleading #950

jangorecki opened this issue Nov 14, 2014 · 11 comments · Fixed by #3457
Assignees
Milestone

Comments

@jangorecki
Copy link
Member

@jangorecki jangorecki commented Nov 14, 2014

Checked on the current latest DT version.
Take the two cols DT where second is list of functions:

DT <- data.table(id=c("a","b"), f=list(function(x) x*2, function(x) x^2), key="id")
lapply(DT,class)
DT[.("a"),f:=list(function(x) x^3)]
#> Error in `[.data.table`(DT, .("a"), `:=`(f, list(function(x) x^3))) : 
#>  RHS of assignment is not NULL, not an an atomic vector (see ?is.atomic) and not a list column.

Expected results would be to update by reference the f column in DT for row id=="a".
Error states that RHS is not a list which is not true. It does not update DT.

sessionInfo()

R version 3.1.2 (2014-10-31)
Platform: x86_64-pc-linux-gnu (64-bit)

locale:
 [1] LC_CTYPE=en_US.UTF-8       LC_NUMERIC=C               LC_TIME=C                  LC_COLLATE=en_US.UTF-8     LC_MONETARY=en_US.UTF-8    LC_MESSAGES=C              LC_PAPER=en_US.UTF-8      
 [8] LC_NAME=C                  LC_ADDRESS=C               LC_TELEPHONE=C             LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C       

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
 [1] devtools_1.6.1   RPostgreSQL_0.4  DBI_0.3.1        logR_1.0.3       Rbitcoin_0.9.3.9 data.table_1.9.5 TTR_0.22-0       xts_0.9-7        zoo_1.7-11      

loaded via a namespace (and not attached):
[1] chron_2.3-45    digest_0.6.4    grid_3.1.2      httr_0.5        jsonlite_0.9.13 lattice_0.20-29 RCurl_1.95-4.3  stringr_0.6.2   tools_3.1.2  
@arunsrinivasan
Copy link
Member

@arunsrinivasan arunsrinivasan commented Nov 14, 2014

You should do:

DT[.("a"),f:=list(list(function(x) x^3))]

The first list() is for the RHS. And you want a list item within it.

@jangorecki
Copy link
Member Author

@jangorecki jangorecki commented Nov 14, 2014

Of course, haven't tried it, sorry for bothering. Maybe improve error message to prevent same question in future(?).

@cwliu007
Copy link

@cwliu007 cwliu007 commented Jul 12, 2018

I encountered the same problem... and solved it by the solution here. Error message indeed for me does not provide clear explanation for the problem.

@jangorecki jangorecki reopened this Jul 13, 2018
@MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented Jul 13, 2018

@genwei007 How about including a gentle reminder:

RHS of assignment is not NULL, not an an atomic vector (see ?is.atomic) and not a list column. Recall that structurally, a list column is a sequence of lists wrapped in list() or .(); if you're trying to create a list column, try wrapping it in list().

Explaining it concisely and generally is a bit tough... we could also point to an FAQ/vignette about creating list columns?

@cwliu007
Copy link

@cwliu007 cwliu007 commented Jul 13, 2018

Thanks for answering. Sorry I am not quite familiar with data.table yet. I have tried using list(), but never think of using list(list()). It may be a point?

@MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented Jul 13, 2018

Have you read the intro vignette?

https://cloud.r-project.org/web/packages/data.table/vignettes/datatable-intro.html

in particular around:

What if we would like to have all the values of column a and b concatenated, but returned as a list column?

If this section is still confusing to you, please let us know. we're in the process of updating that vignette a bit (#2973) and would love your feedback!

@franknarf1
Copy link
Contributor

@franknarf1 franknarf1 commented Jul 13, 2018

Not sure if this is an option, but how about LIST(x) as an alias for list(list(x)) so it's easier to write and read (similar to .() as an alias for list())?

(I use x %>% list %>% list, but it's not great for readability either.)

@MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented Jul 18, 2018

@franknarf1 I like it, but most often my usage looks like .(words = lapply(x, strsplit, ' ')), or else things like .(l_col = list(...), other_col = sum(other_col)), how would the API look for these cases? I'm just worried the use case may be pretty narrow

@franknarf1
Copy link
Contributor

@franknarf1 franknarf1 commented Jul 18, 2018

@MichaelChirico Browsing my code, I only use list columns for tables and always use a := with a single assignment in contrast with your .(v = list(...), ...) or `:=`(v = list(...), ...), like...

# this code is not not reproducible
# but hopefully conveys the idea
indir = "my_indir"
outdir = "my_outdir"

DT = data.table(
  fn = c("p_20180101", "sal_20180101"), 
  nm = c("prices", "transactions"), 
  ofn = c("p.csv", "sal.csv"))

# read in with an fread wrapper
DT[, contents := read_byspec(file.path(indir, fn), nm) %>% list %>% list, by=fn]

# modify some tables
DT[nm == "prices", contents := 
  contents[[1]][!(id %in% c("a", "b", "c"))] %>% list %>% list
]

# write out 
dir.create(outdir)
DT[, write_byspec(contents[[1]], nm, file.path(outdir, ofn)), by=nm]

This table wrangling is done in a wrapper for my main project code which has a "spec" that each csv is tested against on read and write (using vetr to make sure that every column meets basic rules like no NAs, formatting).

So, against my suggestion: it's a pretty narrow use-case and not even in my main code; and in many cases you could do lapply(fn, fread) %>% list, which isn't so onerous and wouldn't be helped by the LIST idea since there's only one %>% list... Similarly, it would not help with the cases you mention.

@UweBlock
Copy link
Contributor

@UweBlock UweBlock commented Jul 31, 2019

@jangorecki
Copy link
Member Author

@jangorecki jangorecki commented Jul 31, 2019

@UweBlock it is already reported in #3626

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.

7 participants