support missing values in measure.vars arg to melt #4720
Conversation
|
EDIT: was a reminder to implement the same logic for id.vars not null, now done. |
Codecov Report
@@ Coverage Diff @@
## master #4720 +/- ##
=======================================
Coverage 99.43% 99.44%
=======================================
Files 73 73
Lines 14460 14469 +9
=======================================
+ Hits 14379 14388 +9
Misses 81 81
Continue to review full report at Codecov.
|
remotes::install_github("Rdatatable/data.table@fix4027")
#> Skipping install of 'data.table' from a github remote, the SHA1 (4c5810c2) has not changed since last install.
#> Use `force = TRUE` to force installation
library(data.table)
dt <- data.table(id = 1, a.1 = 1, a.3 = 3, b.1 = 1, b.2 = 2, b.3 = 3)
melt(
dt, variable.name="ix",
measure.vars=list(a=c("a.1", NA, "a.3"), b=c("b.1", "b.2", "b.3")))
#> id ix a b
#> 1: 1 1 1 1
#> 2: 1 2 NA 2
#> 3: 1 3 3 3 |
|
should the new tests be moved to the "melt section" starting at test number 1035? |
| @@ -105,7 +105,7 @@ SEXP measurelist(SEXP measure, SEXP dtnames) { | |||
| SEXP x = VECTOR_ELT(measure, i); | |||
| switch(TYPEOF(x)) { | |||
| case STRSXP : | |||
| SET_VECTOR_ELT(ans, i, chmatch(x, dtnames, 0)); | |||
| SET_VECTOR_ELT(ans, i, chmatch(x, dtnames, NA_INTEGER)); | |||
There was a problem hiding this comment.
The new chmatch_na function in this PR that I just removed I initially thought was necessary because this PR needed NAs in x not to match to NAs in table. So I started to add bool matchNAtoNA argument to chmatchMain. Then it turned out that just passing nomatch=NA_INTEGER on this line (the only line where chmatch_na was used) instead of nomatch=0, seemed to work and passed the new tests. I left matchNAtoNA as a comment in chmatchMain in case we need to come back to that in future.
@tdhock Please check and let me know if I got anything wrong here. If so, a new test is probably needed please.
| UNPROTECT(protecti); | ||
| return(ans); | ||
| } | ||
|
|
||
| struct processData { | ||
| SEXP RCHK; // a 2 item list holding vars (result of checkVars) and naidx. PROTECTed up in fmelt so that preprocess() doesn't need to PROTECT. To pass rchk, #2865 | ||
| SEXP idcols, valuecols, naidx; // convenience pointers into RCHK[0][0], RCHK[0][1] and RCHK[1] respectively | ||
| int lids, lvalues, lmax, lmin, totlen, nrow; |
There was a problem hiding this comment.
I see that one item (lmin) has been removed from processData. But it looks like you've reviewed the items and commented them which is great (so you know this code better than I do). And no existing tests are changed, just new tests added. So I'm just logging as a comment that I spotted that removed item, and why it looks good to me.
…protect from i,j being used accidentally outside those loops, not really for speed reasons)
Closes #4027
hi @vspinu @Henrik-P @jangorecki @mrdwab tagging you because you commented on the related issue #4027 which I hope this PR will close. In current master I get these errors because NA is unsupported in the measure.vars argument to melt:
I added a couple of simple tests which explain the new behavior implemented in this PR:
It took me a while to read through and understand the fmelt code, and I added some comments which explain the meaning of the different variables/functions. If those comments are not welcome I can delete them.
All tests pass on my computer (Ubuntu 18.04, R-4.0.2).
@MichaelChirico can you code review and maybe suggest additional tests?