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.

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

Closes #1947, exception for gmin/gmax not to fail on ordered factors #2944

Merged
merged 3 commits into from
Aug 13, 2018
Merged

Closes #1947, exception for gmin/gmax not to fail on ordered factors #2944

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2bc0a87
Closes #1947, exception for gmin/gmax not to fail on ordered factors
Jun 20, 2018
6285d74
improve tests
Jun 20, 2018
f11d907
Merge branch 'master' into ordered_factor
mattdowle Aug 13, 2018
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
2 changes: 2 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

#### BUG FIXES

1. `gmin` and `gmax` no longer fail on _ordered_ factors, for which these operations are indeed meaningful, [#1947](https://github.com/Rdatatable/data.table/issues/1947). Thanks to @mcieslik-mctp for identifying and @mbacou for the nudge.

#### NOTES


Expand Down
30 changes: 30 additions & 0 deletions inst/tests/tests.Rraw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11983,6 +11983,36 @@ test(1914.8, identical(dtGet(dt.comp, 1), dt[[1]]))
test(1914.9, identical(dtGet(dt.comp, 'b'), dt$b))
# END port of old testthat tests

# min/max work for _ordered_ factors (not unordered)
lev = letters[1:5]
DT <- CJ(
V2 = letters[6:8],
V1 = factor(lev)
)[-c(4:6, 10:12)]
setkey(DT, NULL)
test(1915.1, DT[ , min(V1)], error = 'not meaningful for factors')
test(1915.2, DT[ , max(V1)], error = 'not meaningful for factors')

## confirming base functionality works
DT[ , V1 := as.ordered(V1)]
test(1915.3, DT[ , min(V1)],
structure(1L, .Label = lev, class = c("ordered", "factor")))
test(1914.4, DT[ , max(V1)],
structure(5L, .Label = lev, class = c("ordered", "factor")))

## make sure GForce is activated
old_optim = options(datatable.optimize = Inf)
test(1914.5, DT[ , min(V1), by = V2],
data.table(
V2 = c("f", "g", "h"),
V1 = structure(1:3, .Label = lev, class = c("ordered", "factor"))
))
test(1914.6, DT[ , max(V1), by = V2],
data.table(
V2 = c("f", "g", "h"),
V1 = structure(3:5, .Label = lev, class = c("ordered", "factor"))
))
options(datatable.optimize = old_optim$datatable.optimize)

###################################
# Add new tests above this line #
Expand Down
4 changes: 2 additions & 2 deletions src/gsumm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ SEXP gmin(SEXP x, SEXP narm)
{
if (!isLogical(narm) || LENGTH(narm)!=1 || LOGICAL(narm)[0]==NA_LOGICAL) error("na.rm must be TRUE or FALSE");
if (!isVectorAtomic(x)) error("GForce min can only be applied to columns, not .SD or similar. To find min of all items in a list such as .SD, either add the prefix base::min(.SD) or turn off GForce optimization using options(datatable.optimize=1). More likely, you may be looking for 'DT[,lapply(.SD,min),by=,.SDcols=]'");
if (inherits(x, "factor")) error("min is not meaningful for factors.");
if (inherits(x, "factor") && !inherits(x, "ordered")) error("min is not meaningful for factors.");
R_len_t i, ix, thisgrp=0;
int n = (irowslen == -1) ? length(x) : irowslen;
//clock_t start = clock();
Expand Down Expand Up @@ -343,7 +343,7 @@ SEXP gmax(SEXP x, SEXP narm)
{
if (!isLogical(narm) || LENGTH(narm)!=1 || LOGICAL(narm)[0]==NA_LOGICAL) error("na.rm must be TRUE or FALSE");
if (!isVectorAtomic(x)) error("GForce max can only be applied to columns, not .SD or similar. To find max of all items in a list such as .SD, either add the prefix base::max(.SD) or turn off GForce optimization using options(datatable.optimize=1). More likely, you may be looking for 'DT[,lapply(.SD,max),by=,.SDcols=]'");
if (inherits(x, "factor")) error("max is not meaningful for factors.");
if (inherits(x, "factor") && !inherits(x, "ordered")) error("max is not meaningful for factors.");
R_len_t i, ix, thisgrp=0;
int n = (irowslen == -1) ? length(x) : irowslen;
//clock_t start = clock();
Expand Down