Skip to content

Align melt with docs for list measure.vars #7257

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.

Sign up for GitHub

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

Closed
Mukulyadav2004 wants to merge 11 commits into from
Closed

Align melt with docs for list measure.vars #7257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8280e2d
use integer indices for measure.vars and error on non scalar aggregates
Mukulyadav2004 Aug 21, 2025
7e02a0d
1037 - 404 and 407
Mukulyadav2004 Aug 21, 2025
5abd761
remove nws item
Mukulyadav2004 Aug 21, 2025
e3b8a64
removed extra multiple value column loop
Mukulyadav2004 Aug 22, 2025
4d283ba
revert fcast modifications
Mukulyadav2004 Aug 22, 2025
879198c
keep non list path
Mukulyadav2004 Aug 23, 2025
ed1712e
revert
Mukulyadav2004 Aug 25, 2025
c0e03ed
delete multiple output column else branch
Mukulyadav2004 Aug 25, 2025
3f0e0a8
Fix test cases
Mukulyadav2004 Aug 25, 2025
c16469b
closed bracket
Mukulyadav2004 Aug 26, 2025
f552d56
EXPECT INTEGER
Mukulyadav2004 Aug 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions inst/tests/tests.Rraw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3359,13 +3359,13 @@ Sep,33.5,19.4,15.7,11.9,0,100.8,100.8,0,12.7,12.7,0,174.1")
test(1037.401, dim(ans<-melt(x, measure.vars=patterns("^y", "^z"))), INT(4,5))
test(1037.402, ans$variable, factor(c("1","1","2","2")))
test(1037.403, dim(ans<-melt(x, measure.vars=patterns("^y", "^z"), variable.factor=FALSE)), INT(4,5))
test(1037.404, ans$variable, c("1","1","2","2"))
test(1037.404, ans$variable, INT(1,1,2,2))
x[, c("y1","z1"):=NA]
test(1037.405, dim(melt(x, measure.vars=patterns("^y", "^z"))), INT(4,5))
test(1037.406, dim(ans<-melt(x, measure.vars=patterns("^y", "^z"), na.rm=TRUE)), INT(2,5))
test(1037.407, ans$variable, factor(c("2","2"), c("1", "2")))
test(1037.408, dim(ans<-melt(x, measure.vars=patterns("^y", "^z"), na.rm=TRUE, variable.factor=FALSE)), INT(2,5))
test(1037.409, ans$variable, c("2","2"))
test(1037.409, ans$variable, INT(2,2))

test(1037.410, melt(data.table(NULL), verbose=TRUE), data.table(NULL),
output="ncol(data) is 0. Nothing to melt")
Expand Down Expand Up @@ -17417,13 +17417,13 @@ exid = data.table(id=1, expected)
test(2182.3, melt(DTid, measure.vars=list(a=c(NA,1), b=2:3), id.vars="id"), exid)
test(2182.4, melt(DTid, measure.vars=list(a=c(NA,"a2"), b=c("b1","b2")), id.vars="id"), exid)
test(2182.5, melt(DT.wide, measure.vars=list(a=c(NA,1), b=2:3), na.rm=TRUE), data.table(variable=factor(2), a=2, b=2))
test(2182.6, melt(DT.wide, measure.vars=list(b=c("b1","b2"))), data.table(a2=2, variable=factor(c("b1","b2")), b=c(1,2)), warning="measure.vars is a list with length=1") # measure.vars named list length=1, #5065
# consistency between measure.vars=list with length=1 and length>1, #5209
test(2182.71, melt(DT.wide, measure.vars=list("a2"), variable.factor=TRUE), data.table(b1=1, b2=2, variable=factor("a2"), value=2), warning="measure.vars is a list with length=1")
test(2182.6, melt(DT.wide, measure.vars=list(b=c("b1","b2"))), data.table(a2=2, variable=factor(c("1","2")), b=c(1,2))) # list yields indices
Copy link
Copy Markdown
Member

@tdhock tdhock Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add these tests from the other pr

test(2182.6, melt(DT.wide, measure.vars=list(b=c("b1","b2"))), data.table(a2=2, variable=factor(c("1","2")), b=c(1,2))) # measure.vars named list length=1, #5065
# consistency between measure.vars=list with length=1 and length>1, #5209
test(2182.71, melt(DT.wide, measure.vars=list("a2"), variable.factor=TRUE), data.table(b1=1, b2=2, variable=factor(1), value=2))
test(2182.72, melt(DT.wide, measure.vars=c("a2"), variable.factor=TRUE), data.table(b1=1, b2=2, variable=factor("a2"), value=2))
test(2182.73, melt(DT.wide, measure.vars=list("a2"), variable.factor=FALSE), data.table(b1=1, b2=2, variable="1", value=2))
test(2182.74, melt(DT.wide, measure.vars=c("a2"), variable.factor=FALSE), data.table(b1=1, b2=2, variable="a2", value=2))
test(2182.75, melt(data.table(a=10, b=20), measure.vars=list(n="a"), variable.factor=FALSE), data.table(b=20, variable="1", n=10))#thanks @mnazarov

# consistency between measure.vars=list with length=1 and length>1 now uses indices for list case, #5209
test(2182.71, melt(DT.wide, measure.vars=list("a2"), variable.factor=TRUE), data.table(b1=1, b2=2, variable=factor(1), value=2))
test(2182.72, melt(DT.wide, measure.vars=c("a2"), variable.factor=TRUE), data.table(b1=1, b2=2, variable=factor("a2"), value=2))
test(2182.73, melt(DT.wide, measure.vars=list("a2"), variable.factor=FALSE), data.table(b1=1, b2=2, variable="a2", value=2), warning="measure.vars is a list with length=1")
test(2182.73, melt(DT.wide, measure.vars=list("a2"), variable.factor=FALSE), data.table(b1=1, b2=2, variable=1L, value=2))
test(2182.74, melt(DT.wide, measure.vars=c("a2"), variable.factor=FALSE), data.table(b1=1, b2=2, variable="a2", value=2))
test(2182.75, melt(data.table(a=10, b=20), measure.vars=list(n="a"), variable.factor=FALSE), data.table(b=20, variable="a", n=10), warning="measure.vars is a list with length=1")#thanks @mnazarov
test(2182.75, melt(data.table(a=10, b=20), measure.vars=list(n="a"), variable.factor=FALSE), data.table(b=20, variable=1L, n=10))#thanks @mnazarov

### First block testing measurev
# new variable_table attribute for measure.vars, PR#4731 for multiple issues
Expand Down
47 changes: 24 additions & 23 deletions src/fmelt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,31 +598,42 @@ SEXP getvarcols(SEXP DT, SEXP dtnames, Rboolean varfactor, Rboolean verbose, str
if (data->lvalues==1 && length(VECTOR_ELT(data->valuecols, 0)) != data->lmax)
internal_error(__func__, "getvarcols %d %d", length(VECTOR_ELT(data->valuecols, 0)), data->lmax); // # nocov
if (isNull(data->variable_table)) {
if ((data->lvalues == 1) & data->measure_is_list) {
warning(_("measure.vars is a list with length=1, which as long documented should return integer indices in the 'variable' column, but currently returns character column names. To increase consistency in the next release, we plan to change 'variable' to integer, so users who were relying on this behavior should change measure.vars=list('col_name') (output variable is column name now, but will become column index/integer) to measure.vars='col_name' (variable is column name before and after the planned change)."));
}
if (!varfactor) {
SET_VECTOR_ELT(ansvars, 0, target=allocVector(STRSXP, data->totlen));
if (data->lvalues == 1) {//one value column to output. TODO #5247 change to !data->measure_is_list
if (data->measure_is_list) {
// Return integer indices for list measure.vars (consistency with docs)
SET_VECTOR_ELT(ansvars, 0, target=allocVector(INTSXP, data->totlen));
int *td = INTEGER(target);
for (int j=0, ansloc=0; j<data->lmax; ++j) {
const int thislen = data->narm ? length(VECTOR_ELT(data->not_NA_indices, j)) : data->nrow;
for (int k=0; k<thislen; ++k) td[ansloc++] = j+1;
}
} else {
// same behavior for vector measure.vars: variable is column names
SET_VECTOR_ELT(ansvars, 0, target=allocVector(STRSXP, data->totlen));
const int *thisvaluecols = INTEGER(VECTOR_ELT(data->valuecols, 0));
for (int j=0, ansloc=0; j<data->lmax; ++j) {
const int thislen = data->narm ? length(VECTOR_ELT(data->not_NA_indices, j)) : data->nrow;
SEXP str = STRING_ELT(dtnames, thisvaluecols[j]-1);
for (int k=0; k<thislen; ++k) SET_STRING_ELT(target, ansloc++, str);
}
} else {//multiple value columns to output.
for (int j=0, ansloc=0, level=1; j<data->lmax; ++j) {
const int thislen = data->narm ? length(VECTOR_ELT(data->not_NA_indices, j)) : data->nrow;
char buff[20];
snprintf(buff, sizeof(buff), "%d", level++); // # notranslate
for (int k=0; k<thislen; ++k) SET_STRING_ELT(target, ansloc++, mkChar(buff));
}
}
} else {// varfactor==TRUE
SET_VECTOR_ELT(ansvars, 0, target=allocVector(INTSXP, data->totlen));
SEXP levels;
int *td = INTEGER(target);
if (data->lvalues == 1) {//one value column to output. TODO #5247 change to !data->measure_is_list
if (data->measure_is_list) {
int nlevel = data->lmax;
levels = PROTECT(allocVector(STRSXP, nlevel)); protecti++;
for (int j=0; j<nlevel; ++j) {
char buff[20];
snprintf(buff, sizeof(buff), "%d", j+1); // # notranslate
SET_STRING_ELT(levels, j, mkChar(buff));
}
for (int j=0, ansloc=0; j<data->lmax; ++j) {
const int thislen = data->narm ? length(VECTOR_ELT(data->not_NA_indices, j)) : data->nrow;
for (int k=0; k<thislen; ++k) td[ansloc++] = j+1;
}
} else { // non-list measure.vars keeps legacy name-based levels
SEXP thisvaluecols = VECTOR_ELT(data->valuecols, 0);
int len = length(thisvaluecols);
levels = PROTECT(allocVector(STRSXP, len)); protecti++;
Copy link
Copy Markdown
Member

@tdhock tdhock Aug 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert these changes and instead use the approach here https://github.com/Rdatatable/data.table/pull/5247/files

Expand All @@ -645,16 +656,6 @@ SEXP getvarcols(SEXP DT, SEXP dtnames, Rboolean varfactor, Rboolean verbose, str
const int thislen = data->narm ? length(VECTOR_ELT(data->not_NA_indices, j)) : data->nrow;
for (int k=0; k<thislen; ++k) td[ansloc++] = md[j];
}
} else {//multiple output columns.
int nlevel=0;
levels = PROTECT(allocVector(STRSXP, data->lmax)); protecti++;
for (int j=0, ansloc=0; j<data->lmax; ++j) {
const int thislen = data->narm ? length(VECTOR_ELT(data->not_NA_indices, j)) : data->nrow;
char buff[20];
snprintf(buff, sizeof(buff), "%d", nlevel + 1); // # notranslate
SET_STRING_ELT(levels, nlevel++, mkChar(buff)); // generate levels = 1:nlevels
for (int k=0; k<thislen; ++k) td[ansloc++] = nlevel;
}
}
setAttrib(target, R_LevelsSymbol, levels);
setAttrib(target, R_ClassSymbol, ScalarString(char_factor));
Expand Down
Loading