BUG: Restrict rv_generic._argcheck() and its overrides from setting members. #9900

pvanmulbregt · 2019-03-05T03:43:23Z

Address incorrect probabilities, broadcasting issues and thread-safety
related to distributions setting member variables inside _argcheck().
Addresses gh-9896, gh-2002, gh-1320, gh-7933.

Add method _get_support(*args) to rv_generic (the default behaviour is to
return sef.a, self.b.) Distributions which have support depending on parameters
need to override this method.
Add overrides for genextreme, kappa4, reciprocal, truncexpon, truncnorm,
binom, bernoulli, hypergeom, boltzmann, randint, rv_sample.
Move code which sets members variables from _argcheck() into _get_support()
and discard any code which actually sets member data.
As most code previously called self._argcheck() anyway before calling
_cdf()/_pdf()/_sf()/..., the impact on performance is perhaps minimal.
This fixes threading concerns, because the (global) distribution is no longer
being changed every call.
Fixes broadcasting issues because the _cdf(x, *args, **kwargs)/... calls now
actually use *args, which has the correct size.

ev-br · 2019-03-16T13:04:17Z

Looks good on a quick browse.
Performance is probably not a big concern indeed, the overhead is huge already, it's unlikely to get significantly worse.
Note that the framework won't be fully thread-safe anyway, because of the self._size dance in rvs.

Could you expand a bit: you mention this addresses several issues, what is their status with this change? Are they fixed? In that case, it'd be helpful to add specific regression tests, with references to GH issues.
If cooking up a threading regression test is not too difficult, it'd be nice, too.

pvanmulbregt · 2019-03-16T21:04:11Z

To be explicit, this PR fixes:

scipy.stats._argcheck sets (not just checks) values #9896 (broadcasting, threading)
stats.truncnorm fails when providing values outside truncation range. #7933 (broadcasting)
members set in scipy.stats.distributions.##._argcheck (Trac #1477) #2002 (broadcasting)
It fixes the first example in scipy.stats.distribution: problem with self.a, self.b if they depend on parameters (Trac #793) #1320 (broadcasting)
c=2.62;stats.genextreme.ppf(0.5,np.array([[c],[c+0.5]]))
The later examples such as
c=2.62;stats.genextreme.moment(5,np.array([c,c+0.5]))
are not addressed.
-It appears that some examples in scipy.stats.distribution: problem with self.a, self.b if they depend on parameters (Trac #793) #1320 which did work in 2013 (such as
c=2.62;stats.genextreme.moment(2,np.array([c,c+0.5])))
stopped working some time ago (E.g. Py2.7, Scipy 1.13.0), generating errors such as
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

The most recent pushes added named tests for the 4 Issues. An errorful threading situation is simulated in one of these tests. It fails prior to gh-9990, passes with it.

rlucas7

I still need to digest all the code in this PR, will take some time. In the meanwhile, inequality in my comment was broken in irwinhall pr so pointing that out.

rlucas7 · 2019-03-29T02:37:43Z

scipy/stats/_distn_infrastructure.py

-        if self.b < np.inf:
-            right = self.b
+        _a, _b = self._get_support(*args)
+        if _a > -np.inf:


Here if arguments are array_like the truthyness of the inequality is ambiguous, similar for _b just beneath.
For example, in case of irwinhall (or even binom support and n parameter is a 2 entry array_like.

Interesting. Previously self.a and self.b were fixed numbers. Now they are dependent on*args, which might not be specifying a single distribution, but a family of them. That will also impact the determination of left and right in the block just below this.

Commit 36a9e03 addressed this issue.

Commit 36a9e03 addressed this issue.

Yes.

pvanmulbregt · 2019-04-03T02:50:23Z

The test failures appear to be unrelated.

The ci/circleci: pypy3 test failed immediately with a 404 error getting the Debian security updates.
The ci/circleci: build_docs target built the html and latex but failed the pdflatex with Invalid fontname [FreeSerif.otf]/OT', contains '['`

ilayn · 2019-04-03T05:44:58Z

build_docs should go away if you rebase it

…embers. Address incorrect probabilities, broadcasting issues and thread-safety related to distributions setting member variables inside _argcheck(). Addresses scipygh-9896, scipygh-2002, scipygh-1320, scipygh-7933. Add method _get_support(*args) to rv_generic (the default behaviour is to return sef.a, self.b.) Distributions which have support depending on parameters need to override this method. Add overrides for genextreme, kappa4, reciprocal, truncexpon, truncnorm, binom, bernoulli, hypergeom, boltzmann, randint, rv_sample. Move code which sets members variables from _argcheck() into _get_support() and discard any code which actually sets member data. As most code previously called self._argcheck() anyway before calling _cdf()/_pdf()/_sf()/..., the impact on performance is perhaps minimal. This fixes threading concerns, because the (global) distribution is no longer being changed every call. Fixes broadcasting issues because the _cdf(x, *args, **kwargs)/... calls now actually use *args, which has the correct size.

rv_sample, rv_hist are not the same as rv_discrete/rv_continuous; they create separate instances so setting of member variables is necessary.

Added stats/tests/test_distributions::test_retrieving_support() to test <dist>.support(*args, loc, scale) for all distributions.

…inuous_basic.py. Added test_nomodify_gh9900_regression() to simulate thread interleaving. Added test_broadcast_gh9990_regression() to validate broadcast for out-of-support arguments, and to check support endpoints.

…h-1320. Added checks for broadcasting in stats/tests/test_continuous_basic.py: test_broadcast_gh7933_regression(), test_gh2002_regression(), and test_gh1320_regression().

pvanmulbregt · 2019-04-04T02:27:43Z

Rebase fixed the build_docs and the pypy3 target also succeeded.

ev-br · 2019-04-14T09:17:38Z

Am adding the 1.3.0 milestone, if only to indicate that I'm going to try to give it a final review before branching 1.3.0. The decision is RM's, of course.

rlucas7 · 2019-04-15T23:48:40Z

scipy/stats/_distn_infrastructure.py

+    def _get_support(self, *args):
+        """Return the support of the (unscaled, unshifted) distribution.
+
+        *Must* be overridden by distributions which have support dependent


Is the intent that when a new distribution is added and the distribution's support depends on a parameter value, that the value is set in the _get_support method that is overridden in the child class?

If so maybe state that explicitly here? I read this as you must override the function with a distribution specific one, but not that the code in the _get_support should override the support limits.

If not, then where is the developer to place the overridden a and b parameters (for location and scale)

It is not the intent to ever set self.a or self.b for such a distribution. The issue is that a single instance of the class truncexpon is shared amongst all users of the truncexpon distribution, so setting it in one circumstance sets it for all. Even a frozen distribution is not actually frozen --- what are frozen are the constructor arguments so that it can use the distribution at some later time.

self.a and self.b do not need to get changed for location and scale --- handling that happens automatically in the methods of rv_continuous, such as rv_continuous.cdf etc.

ok, so I understood correctly. I'd recommend adding a line here that states explicitly not to use _get_support to set the values either. I'm worried the message will not be explicit enough for future distribution creators. Maybe something like:

"The self.a and self.b limits should not be set by this method, rather this is a utility method that returns the support limits for use in other distribution class methods. "

Or something similar if you prefer a different wording.

I modified the docstring of rv_generic and rv_continuous to be more explicit about the need for overriding _get_support, and modified the docstring of _get_support to be explicit on not permitting any modification of any members of the class.

Thanks for that, that will make it easier on new developers coming into the scipy.stats submodule in the future (and me when I need to remember this again).

rlucas7 · 2019-04-17T00:25:24Z

Was able to look at this a bit last night, here are my findings:

confirmed on python 3.6 on my box that this fixes part of gh-1320 and all of gh-7933.

Here is session for gh-7933:

Python 3.6.7 |Anaconda, Inc.| (default, Oct 23 2018, 14:01:38) 
[GCC 4.2.1 Compatible Clang 4.0.1 (tags/RELEASE_401/final)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import numpy as np
>>> from scipy.stats import truncnorm
>>> truncnorm.logpdf(
...     np.array([3.0, 2.0, 1.0]),
...     a=(1.5-np.array([6.0, 5.0, 4.0]))/3.0,
...     b=np.inf,
...     loc=np.array([6.0, 5.0, 4.0]),
...     scale=3.0
... )
array([-2.44840737, -2.38781507,        -inf])

and here is the session for gh-1320

(scipy-dev) Lucass-MacBook:scipy rlucas$ python
Python 3.6.7 |Anaconda, Inc.| (default, Oct 23 2018, 14:01:38) 
[GCC 4.2.1 Compatible Clang 4.0.1 (tags/RELEASE_401/final)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import scipy.stats as ss
>>> c=2.62
>>> c
2.62
>>> ss.genextreme.ppf(0.5,np.array([[c],[c+0.5]]))
>>> import numpy as np
>>> ss.genextreme.ppf(0.5,np.array([[c],[c+0.5]]))
array([[0.23557562],
       [0.21836694]])
>>> ss.genextreme._ppf(0.5,np.array([c,c+0.5]))
array([0.23557562, 0.21836694])
>>> ss.genextreme._mom0_sc(2,np.array([c,c+0.5]))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/rlucas/scipy-dev/scipy/scipy/stats/_distn_infrastructure.py", line 1660, in _mom0_sc
    args=(m,)+args)[0]
  File "/Users/rlucas/scipy-dev/scipy/scipy/integrate/quadpack.py", line 337, in quad
    flip, a, b = b < a, min(a, b), max(a, b)
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()
>>> c
2.62
>>> ss.genextreme.moment(2,np.array([c,c+0.5]))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/rlucas/scipy-dev/scipy/scipy/stats/_distn_infrastructure.py", line 1160, in moment
    if not (self._argcheck(*args) and (scale > 0)):
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()
>>> ss.genextreme.moment(5,np.array([c,c+0.5]))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/rlucas/scipy-dev/scipy/scipy/stats/_distn_infrastructure.py", line 1160, in moment
    if not (self._argcheck(*args) and (scale > 0)):
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()
>>> 
(scipy-dev) Lucass-MacBook:scipy rlucas$

So far so good, will keep looking to see what I find on other addressed issues.

…port.

…_support().

rlucas7

I took another look at this tonight. This is a nice fix to an issue. The PR addresses the issue for univariate continuous and discrete distributions. The issue does not exist with multivariate distributions.

@pvanmulbregt LGTM, hopefully @ev-br (or another scipy member) can take a look before the 1.3.0 release drop date (which is soon). I would love to have this in the release and unblock the irwinhall dev work.

rlucas7 · 2019-04-23T01:01:50Z

scipy/stats/_distn_infrastructure.py

-        if self.b < np.inf:
-            right = self.b
+        _a, _b = self._get_support(*args)
+        if _a > -np.inf:


Commit 36a9e03 addressed this issue.

Yes.

rgommers · 2019-04-23T09:53:47Z

I have no time to look in detail before branching; I suggest to wait for @ev-br's final review for now and if he doesn't get to it then merge as is, since it's a clear improvement and all open comments have been addressed.

ev-br · 2019-04-23T11:31:24Z

Don't wait for me, there's no point holding this PR.

rgommers · 2019-04-23T11:35:01Z

Okay, in it goes!

Thanks @pvanmulbregt and all reviewers!

tylerjereddy · 2019-04-23T17:45:04Z

I went through and manually closed the issues that were claimed to be addressed in the initial PR description, and added 1.3.0 milestones to those issues. For one of them, based on the extended discussion above, it looks like this PR only provides a partial fix, so maybe another issue should be opened to split off residual problems.

tylerjereddy · 2019-04-23T17:49:12Z

Added crude "release note" to end of other changes section. Feel free to improve or just remove if not really appropriate there.

pvanmulbregt · 2019-04-23T22:54:23Z

I went through and manually closed the issues that were claimed to be addressed in the initial PR description, and added 1.3.0 milestones to those issues. For one of them, based on the extended discussion above, it looks like this PR only provides a partial fix, so maybe another issue should be opened to split off residual problems.

To be explicit (and save me having to think too hard some time in the future), I presume you are referring to gh-1320. If so, (see #9900 (comment) this PR (and its patches) indeed only fixes the first example; the second example is still broken, and the third example broke in the intervening years.

tylerjereddy · 2019-04-23T23:22:18Z

@pvanmulbregt correct

pvanmulbregt added the scipy.stats label Mar 5, 2019

ev-br mentioned this pull request Mar 26, 2019

ENH: Add the Irwinhall distribution #9963

Closed

rlucas7 reviewed Mar 29, 2019

View reviewed changes

pvanmulbregt added 6 commits April 3, 2019 21:44

Updated rv_sample, rv_hist: setting self.a, self.b allowed.

2725b74

rv_sample, rv_hist are not the same as rv_discrete/rv_continuous; they create separate instances so setting of member variables is necessary.

TST: Added a test of dist.support(*args, loc, scale).

dbc668f

Added stats/tests/test_distributions::test_retrieving_support() to test <dist>.support(*args, loc, scale) for all distributions.

TST: Added regression tests for scipygh-7933, scipygh-2002 and scipyg…

b4bdd28

…h-1320. Added checks for broadcasting in stats/tests/test_continuous_basic.py: test_broadcast_gh7933_regression(), test_gh2002_regression(), and test_gh1320_regression().

BUG: Fixed a broadcast bug related to support.

36a9e03

pvanmulbregt force-pushed the argcheck_noset branch from 70d6e04 to 36a9e03 Compare April 4, 2019 01:44

ev-br added this to the 1.3.0 milestone Apr 14, 2019

pvanmulbregt mentioned this pull request Apr 14, 2019

ENH: Added exact computation for Kolmogorov-Smirnov two-sample test. #9753

Merged

rlucas7 reviewed Apr 15, 2019

View reviewed changes

pvanmulbregt added 3 commits April 21, 2019 14:59

DOC: Updated docstring of rv_generic/rv_continuous regarding _get_sup…

3ca76cb

…port.

DOC: Be even more explict about not changing member variables in _get…

45f1a7e

…_support().

DOC: Fixed documentation to be reST compliant.

e2c2c17

rlucas7 reviewed Apr 23, 2019

View reviewed changes

rgommers merged commit 1ed24d6 into scipy:master Apr 23, 2019

rgommers added the maintenance Items related to regular maintenance tasks label Apr 23, 2019

tylerjereddy mentioned this pull request Apr 23, 2019

scipy.stats._argcheck sets (not just checks) values #9896

Closed

ev-br mentioned this pull request Apr 23, 2019

distribution methods add tmp #2823

Closed

chrisb83 mentioned this pull request Aug 6, 2019

add test to distributions for multiple distribution parameters (Trac #938) #1465

Closed

WarrenWeckesser mentioned this pull request Nov 5, 2019

truncnorm pdf doesn't work for multidimensional arrays #11008

Closed

gavinevans mentioned this pull request Nov 21, 2019

Specify the distribution when creating percentiles/probabilities from the mean and variance metoppv/improver#1008

Merged

2 tasks

pvanmulbregt deleted the argcheck_noset branch December 29, 2019 21:28

pvanmulbregt mentioned this pull request Jan 10, 2020

stats.truncnorm.rvs 100x slower in v1.4.x than v1.3.3 #11299

Closed

pvanmulbregt mentioned this pull request Jan 20, 2020

MAINT: Remove self._size (and self._random_state) from stats distributions. #11394

Merged

ev-br mentioned this pull request Feb 20, 2020

MAINT: stats: remove an _argcheck call #11560

Merged

pvanmulbregt mentioned this pull request May 4, 2020

ENH (WIP): Adds Poisson binomial distribution support (See #6000) #12000

Closed

WarrenWeckesser mentioned this pull request May 21, 2020

Broadcasting fails in hypergeom.cdf #7985

Closed

Uh oh!

BUG: Restrict rv_generic._argcheck() and its overrides from setting members. #9900

BUG: Restrict rv_generic._argcheck() and its overrides from setting members. #9900

Uh oh!

Conversation

pvanmulbregt commented Mar 5, 2019

Uh oh!

ev-br commented Mar 16, 2019

Uh oh!

pvanmulbregt commented Mar 16, 2019

Uh oh!

rlucas7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pvanmulbregt commented Apr 3, 2019

Uh oh!

ilayn commented Apr 3, 2019

Uh oh!

pvanmulbregt commented Apr 4, 2019

Uh oh!

ev-br commented Apr 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rlucas7 commented Apr 17, 2019

Uh oh!

rlucas7 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgommers commented Apr 23, 2019

Uh oh!

ev-br commented Apr 23, 2019

Uh oh!

rgommers commented Apr 23, 2019

Uh oh!

tylerjereddy commented Apr 23, 2019

Uh oh!

tylerjereddy commented Apr 23, 2019

Uh oh!

pvanmulbregt commented Apr 23, 2019

Uh oh!

tylerjereddy commented Apr 23, 2019

Uh oh!

Uh oh!

rlucas7 left a comment •

edited

Loading