berndbischl / BBmisc

Other
20 stars 7 forks source link

New function getBestIndex #62

Closed jakobbossek closed 8 years ago

jakobbossek commented 8 years ago

Simple wrapper for get{Min,Max}Index.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.02%) to 90.041% when pulling f3c442c4ff35c982a424a82ed6ba389bf4183c9a on feature_getBestIndex into f22544f76356e4040759a0e538a97c8f63b07ad5 on master.

berndbischl commented 8 years ago

thx looks good.

one mini question: isnt it in these cases better not to use dotargs but to explicitly write down the args? (na.rm and so on).

for command completion and clearness sake?

(i would also merge without this)

jakobbossek commented 8 years ago

one mini question: isnt it in these cases better not to use dotargs but to explicitly write down the > args?

What do you mean by these cases? Functions with few parameters?

for command completion and clearness sake? (i would also merge without this)

Well, yeah these dots always make it harder for code completation, but at least clearness is given by the docs.

If you want, I can drop the dotargs.

berndbischl commented 8 years ago

What do you mean by these cases? Functions with few parameters?

i am not sure. i think so :) i just thought about it here.

If you want, I can drop the dotargs.

you decide. i think it might be better, but will merge both versions

jakobbossek commented 8 years ago

Then merge plz. We can still change this later.

jakobbossek commented 8 years ago

I will update NEWS in the master branch afterwards.

jakob-r commented 8 years ago

Just out of curiosity, as getMaxIndex is C and supposedly fast is then -as.numeric(x) not a bad practice as it generates some overhead by copying and stuff?

library(microbenchmark)
library(ggplot2)
library(BBmisc)
x = rnorm(10000)
res = microbenchmark(getMinIndex(x), getMaxIndex(x), times = 1000, control = list(warmup = 1000))
autoplot(res)

image

berndbischl commented 8 years ago

Just out of curiosity, as getMaxIndex is C and supposedly fast is then -as.numeric(x) not a bad practice as it generates some overhead by copying and stuff?

yes. but if you leave that insight here in the closed PR it will be lost.