-
Notifications
You must be signed in to change notification settings - Fork 992
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new chmatch_na
function in this PR that I just removed I initially thought was necessary because this PR needed NA
s in x
not to match to NA
s 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?