orderNumeric doesn't support na.last

[sgibb]

The current implementation of orderNumeric (or its C implementation of Double_order/_get_order_of_double_array) doesn't support the na.last argument.

I used orderNumeric as fast replacement for base::order and expecting it to work identical because of the same API.

MSnbase:::orderNumeric(c(NA, 1), na.last=TRUE)
# [1] 1 2
base::order(c(NA, 1), na.last=TRUE)
# [1] 2 1

If it is too hard to implement it I would prefer at least a warning if this argument is set by the user or remove it completely (because it is ignored anyway).

[jorainer]

I'll check into that.

[jorainer]

Actually, I think we should remove (or deprecate) the orderInteger, orderNumeric and sortNumeric R functions since in R >= 3.3 it is possible to select the sorting method, and method = "radix" beats any other option. I added the R functions because I thought they might be useful, but in the end I'm just using their C function in the Spectrum1 and Spectrum2 C-level constructors.

library(testthat)
library(MSnbase)
library(microbenchmark)

dat <- rnorm(100000)
## Add some ties:
dat[sample(1:length(dat), 10)] <- 5.4
dat[sample(1:length(dat), 10)] <- 1.123
ordr_1 <- base::order(dat, method = "auto")
ordr_2 <- base::order(dat, method = "shell")
ordr_3 <- base::order(dat, method = "radix")
ordr_4 <- MSnbase:::orderNumeric(dat)
expect_identical(ordr_1, ordr_2)
expect_identical(ordr_2, ordr_3)
expect_identical(ordr_3, ordr_4)

microbenchmark(base::order(dat, method = "auto"),
               base::order(dat, method = "shell"),
               base::order(dat, method = "radix"),
               MSnbase:::orderNumeric(dat))

Unit: milliseconds
                               expr      min        lq      mean    median
  base::order(dat, method = "auto")  2.88673  3.048574  3.555444  3.274347
 base::order(dat, method = "shell") 18.07733 19.580625 20.769978 20.550710
 base::order(dat, method = "radix")  2.83596  3.003941  3.519363  3.185912
        MSnbase:::orderNumeric(dat) 14.87096 16.177153 17.342168 16.844887
        uq       max neval cld
  3.788631  6.302562   100 a  
 21.683892 30.198646   100   c
  3.723653  7.537382   100 a  
 17.942919 30.880739   100  b 

[jorainer]

Actually, for backward compatibility, it's better not to use the method parameter in the sort and order functions, since these are not present in R < 3.3. The default seems to use anyway radix sort, if available (see also #180).

[sgibb]

IMHO there is no need to deprecated this methods. They are hidden (not exported by NAMESPACE), not documented and not used anywhere. So I vote for removing them now.

[jorainer]

OK, I'll do - just didn't want to break anything if you have them somewhere in your code.

[sgibb]

Thanks, that's very kind of you but I already replaced everything by base::order. I will close this issue.