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

NEWS.md

@@ -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#issuecomment-398212372). Thanks to @mcieslik-mctp for identifying and @mbacou for the nudge. 
+
 #### NOTES
 
 

inst/tests/tests.Rraw

@@ -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)
+DT <- data.table(
+  V1=factor(rep(c("a","b"), 10), levels=c("a", "b")),
+  V2=rep(c("c","d", "e", "f"), 5)
+)
+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 = c("a", "b"), class = c("ordered", "factor")))
+test(1914.4, DT[ , max(V1)],
+     structure(2L, .Label = c("a", "b"), 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("c", "d", "e", "f"),
+       V1 = structure(c(1L, 2L, 1L, 2L), .Label = c("a", "b"),
+                      class = c("ordered", "factor"))
+     ))
+test(1914.6, DT[ , max(V1), by = V2],
+     data.table(
+       V2 = c("c", "d", "e", "f"),
+       V1 = structure(c(1L, 2L, 1L, 2L), .Label = c("a", "b"),
+                      class = c("ordered", "factor"))
+     ))
+options(datatable.optimize = old_optim$datatable.optimize)
 
 ###################################
 #  Add new tests above this line  #

src/gsumm.c

@@ -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();
@@ -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();