Shaunagm/bugfix/discrete

[shaunagm]

No description provided.

[ev-br]

This PR replaces gh-3195.

[ev-br]

for continuous distributions, pdf is the probability density rather than a probability itself, it does not need to be in [0, 1].

[shaunagm]

Removed the changes to pdf, and rebased the commits while I was at it so the commit history's cleaner. I also added a working test but when running the suite, I found 8 failures apparently induced by the clipping changes.

[ev-br]

Sorry for a delay. The test failures I see are twofold:

• the failure in vonmises is a bit weird. vonmises distribution, as implemented in stats, is a bit special: it's periodic with the support extending to the whole real axis, so that the integral of the pdf over the support is not equal to one etc. Now, the cdf, being an integral of the pdf, does not go to one at the right-hand edge of the support, hence clipping breaks it. For historical reasons, this is to stay [there is also vonmises_line, which has the support of (-pi, pi)]. Therefore, clipping of the cdf and sf of continuous distributions will not work, and my original suggestion to add it was useless. Can you roll it back?
• Another failure, in TestHypergeom is due to the way you've set up the test. First of all, better make it a separate function, and add a comment explaining where the numbers came from. Now, you're insisting that the value equals one which is typically not what you want with floating-point numbers. Here you're asserting that the result is not larger than one, so I'd write it roughly like so:

# for some values of parameters, hypergeom cdf was >1, see gh-2238
def test_cdf_above_one(self):
   assert_( 0 <= stats.hypergeom.cdf(30,13397950,4363,12390) <= 1.0 )

(notice the use of assert_ from numpy)

[shaunagm]

I've made your suggested changes, which allowed the tests to pass.

I'm curious what thought process led you to "drop clipping for cdf and sf for continuous distributions" rather than "test vonmises separately"? Is there a general preference to, within broad categories, treat distributions the same?

[ev-br]

This isn't about testing, this is about not breaking the previous behavior: If you then plot vonmises.cdf, you'll see that it's not confined to [0, 1]. Then, test_vonmises_periodic tests periodicity of cdf modulo one. So, the previous behavior is to have vonmises distribution to have unbounded support and a special trick to get a proper cdf. Just clipping it would break the established behavior and this is undesirable.
Re thought process: well, I started from trying to understand why the test fails, and went from there.

[ev-br]

Merged it, thanks @shaunagm!
Notice that I squashed it into a single commit.