Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

fix sample-binomial and sample-neg-binomial ignoring/mangling arguments #109

Closed
wants to merge 2 commits into from

2 participants

@lionandoil

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
@alexott
Owner

Hi Kevin

Can you start discussion of this in incanter mailing list? It would be nice to collect different opinions on this, because breaking change could affect many peoples...

thank you

@lionandoil

No opinions to be heard in the Google group - in any case, I've got a non-signature-changing fix ready in my fork. The Travis builds seem to be broken at the moment due to dependency issues(?) - should I make a pull request anyway?

Best!

@alexott
Owner

Yes, please do new pull request, and we'll see what's wrong with travis and will try to fix it

@lionandoil lionandoil closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
8 examples/probability_plots.clj
@@ -370,16 +370,16 @@
;; make box-plots for each of the Binomial distributions
-(doto (box-plot (sample-binomial 1000 :prob 1/2 :size 20)
+(doto (box-plot (sample-binomial 1000 20 1/2)
:title "Binomial Boxplot"
:legend true)
- (add-box-plot (sample-binomial 1000 :prob 0.7 :size 20))
- (add-box-plot (sample-binomial 1000 :prob 1/2 :size 40))
+ (add-box-plot (sample-binomial 1000 20 0.7))
+ (add-box-plot (sample-binomial 1000 40 1/2))
view)
;; make a histogram of a sample of 1000 Exponential deviates
-(view (histogram (sample-binomial 1000 :prob 1/2 :size 20)
+(view (histogram (sample-binomial 1000 20 1/2)
:title "Binomial Histogram"
:nbins 10))
View
34 modules/incanter-core/src/incanter/stats.clj
@@ -959,7 +959,7 @@
:rate (default 1)
See also:
- pdf-exp, and cdf-exp
+ pdf-exp and cdf-exp
References:
http://incanter.org/docs/parallelcolt/api/cern/jet/random/tdouble/Exponential.html
@@ -1157,24 +1157,18 @@
" Returns a sample of the given size from a Binomial distribution.
Same as R's rbinom
- Options:
- :size (default 1)
- :prob (default 1/2)
-
See also:
- cdf-binomial and sample-binomial
+ pdf-binomial and cdf-binomial
References:
http://incanter.org/docs/parallelcolt/api/cern/jet/random/tdouble/Binomial.html
http://en.wikipedia.org/wiki/Binomial_distribution
Example:
- (sample-binomial 1000 :prob 1/4 :size 20)
+ (sample-binomial 1000 20 1/4)
"
-([^Integer size & {:keys [size prob] :or {size 1 prob 1/2}}]
- (if (= size 1)
- (Binomial/staticNextInt size prob)
- (for [_ (range size)] (Binomial/staticNextInt size prob)))))
+([^Integer size ^Integer n p]
+ (repeatedly size #(Binomial/staticNextInt n p))))
@@ -1267,7 +1261,7 @@
:lower-tail (default true)
See also:
- cdf-poisson and sample-poisson
+ pdf-poisson and sample-poisson
References:
http://incanter.org/docs/parallelcolt/api/cern/jet/random/tdouble/Poisson.html
@@ -1353,7 +1347,7 @@
:lower-tail? (default true)
See also:
- cdf-neg-binomial and sample-neg-binomial
+ pdf-neg-binomial and sample-neg-binomial
References:
http://incanter.org/docs/parallelcolt/api/cern/jet/random/tdouble/NegativeBinomial.html
@@ -1373,15 +1367,10 @@
-;; TODO: may be this is a bug, that we have 2 size? in param and in options
(defn sample-neg-binomial
" Returns a sample of the given size from a Negative Binomial distribution.
Same as R's rnbinom
- Options:
- :size (default 10)
- :prob (default 1/2)
-
See also:
pdf-neg-binomial and cdf-neg-binomial
@@ -1390,13 +1379,10 @@
http://en.wikipedia.org/wiki/Negative_binomial_distribution
Example:
- (sample-neg-binomial 1000 :prob 1/2 :size 20)
+ (sample-neg-binomial 1000 20 1/2)
"
-([^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)))))
-
+([^Integer size ^Integer n p]
+ (repeatedly size #(NegativeBinomial/staticNextInt n p))))
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Something went wrong with that request. Please try again.