liebke/incanter — pull request #109 by lionandoil (2012-12-20 19:34:44+00:00)
Title: fix sample-binomial and sample-neg-binomial ignoring/mangling arguments

The current implementation of sample-binomial has two arguments named 'size' (one for the incanter sample size, one for the parameter 'n' of the binomial distribution), which means that at the moment the sample size parameter is always overwritten and set to the same value as n (or 1 by default):

=> (sample-binomial 10) ; expect a seq of 10 0's and 1's
1
=> (sample-binomial 1 :size 10) ; expect a single number between 0 and 10
(4 9 5 7 4 8 4 6 6 6)

Current function signature:

(defn sample-binomial
([^Integer size & {:keys [size prob] :or {size 1 prob 1/2}}]
The same problem applies to sample-neg-binomial:

;; TODO: may be this is a bug, that we have 2 size? in param and in options
(defn sample-neg-binomial
([^Integer nsize & {:keys [size prob] :or {size 10 prob 1/2}}]
(if (= size 1)
(NegativeBinomial/staticNextInt size prob)
(for [_ (range size)] (NegativeBinomial/staticNextInt size prob)))))
Note how *nsize* has been named differently by whoever spotted the issue but its value is never used within the function.

Possible ways to fix this:

1. change the variable name of the unnamed argument - fixes the behaviour while leaving the interface unchanged
2. change the named parameter :size to the more idiomatic :n (maybe also change :probs to :p?) - changes both the behaviour as well as the keyword arguments' name to signify the change in behaviour
3. the corresponding function signature in R is **rbinom(n, size, prob)** (no default args!) so maybe sample-binomial should also be changed to take 3 mandatory arguments. My idea was to add this function signature to sample-binomial, *while leaving the old (buggy) behaviour in place* for compatibility, but unfortunately the current (variable argument) keyword-based implementation isn't compatible with fixed signatures of more than one argument ($#&?%!!) which leaves us with another option:
4. break compatibility completely by replacing the current function signature with a 3-argument one

The changes for option 4 are attached - I know that a signature change is kind of a big deal but I'm not sure if anyone is actually using this function given how off its behaviour is and the fact that no one has reported it yet (the only calls to the function in the examples were passing the result straight to boxplot without caring about what sample size was actually returned). Both functions are also deviant in their behaviour of returning just one value when called with size 1 in contrast to other functions in the sample-* family which return a seq with one element.

=> (sample-binomial 1) ; expect a seq with one element
0