Increase logcdf coverage for invalid parameter values #4421
Conversation
ricardoV94
force-pushed
the
increase_logcdf_coverage_2
branch
from
7cd42b5 to
83014e8
Compare
pymc3/tests/test_distributions.py
Outdated
| @@ -807,7 +885,8 @@ def test_chi_squared(self): | |||
| lambda value, nu: sp.chi2.logpdf(value, df=nu), | |||
| ) | |||
|
|
|||
| @pytest.mark.xfail(reason="Poor CDF in SciPy. See scipy/scipy#869 for details.") | |||
| # TODO: Is this still needed? It passes locally. | |||
| # @pytest.mark.xfail(reason="Poor CDF in SciPy. See scipy/scipy#869 for details.") | |||
There was a problem hiding this comment.
Usually, an XFAIL should error if the test does NOT fail.
In any case, these commented lines must be dealt with before merging.
There was a problem hiding this comment.
I probably misinterpreted something somewhere, but this test was not failing (nor failing because it was not failing) on my machine. Anyway, it should soon be cleared up when the tests conclude in here.
Fair enough! In most cases I simply had to borrow the same bounds as in the logp methods, except for the bound on the input |
ricardoV94
force-pushed
the
increase_logcdf_coverage_2
branch
from
2596855 to
9c5aa05
Compare
|
Unrelated failing test. I will wait for reviews before forcing the tests to rerun. The last commit was just a change in the docstrings, and the tests before it were fine. |
|
|
let's open an issue about that then. @ricardoV94 what about the unchecked boxes above? |
Basically it seems that in the pytest context we are still triggering those annoying C-level assections in the Theano Gamma functions mentioned in aesara-devs/aesara#224 # In pytest context
pymc3_dist = pm.Gamma
valid_value = array(0.01)
test_params = {'alpha': -1.0, 'beta': array(0.5)}
invalid_dist = pymc3_dist.dist(**test_params)
assert_equal(
invalid_dist.logcdf(valid_value).tag.test_value, # This triggers the C level assertion. Is it evaluating greedily or using cached results?
-np.inf,
err_msg=str(test_params),
) double GammaP(double, double): Assertion `(n > 0) && (x >= 0)' failed.Actually it seems that simply calling logcdf is enough to trigger the exception invalid_dist.logcdf(valid_value)This previous PR #4393 was supposed to avoid this by using # in normal iPython context
invalid_dist.logcdf(valid_value).eval()
array(-inf)Unless someone has a good clue at what is going on, I would suggest to just keep those tests disabled, as nothing was changed about those functions. I cannot use |
|
@ricardoV94 helpful comment & waiting for aesara-devs/aesara#224 sounds good. |
ricardoV94
force-pushed
the
increase_logcdf_coverage_2
branch
from
723d8d4 to
8358a97
Compare
|
Yes (I don't know how to) |
|
Just click on the big green button below 👇 Stay on the "Squash and merge" option, and add that it "Closes #4399" in the commit message -- and then celebrate 🍾 |
|
Haha maybe next time! Edit: I thought someone had done it already! Done |
This PR extends the work started in the #4393 PR, and addresses the issue that most logcdf methods for continuous distributions were not checking for invalid parameteres (closes #4399 issue).
The main change is that
test_distributions.py::check_logcdfmethod now tests that distributions return-infwhen given invalid parameters (i.e., one unit below or above a finite domain edge). This is done for one invalid parameter at a time.This extended test detected failures in all the continuous distribution logcdf methods mentioned in #4399 (and some more), as well as an issue in the discrete
BetaBinomialdistribution that I had missed before. The solution to most cases was to simply wrap the returned result in aBound.For the
Uniform,TriangularandDiscreteUniformdistributions it does not make sense to test values outside of the "Domain edges" since these are set arbitrarily (so that the upper and lower parameters do not clash). I added an optional argumentskip_paramdomain_outside_edge_testthat skips this step for these distributions.The
ExGaussiandistribution was not being tested withcheck_logcdfprobably because there is no Scipy implementation. To have it covered in the new tests I added the optional argumentskip_paramdomain_inside_edge_testwhich skips the pymc3 vs scipy comparison for valid parameters / values (i.e., the default behavior ofcheck_logcdf).Since this PR and #4393 added considerable complexity to the
check_logcdfmethod, I went ahead and documented it as best as I could. This will hopefully make it easier to maintain in the future.Two issues remain:
GammaandInverseGammafail mysteriously with aFatal Python error: Abortedunless I skip the new added tests withskip_paramdomain_outside_edge_test=True.I could not understand why this is the case, since the functions seem to work fine when I test them with the exact same values in iPython. Maybe it's something to do with using theUpdate: Issue is explained below and it should go away once Return NaN in C implementations of SciPy Ops aesara-devs/aesara#224 is closed.tag.test_valuevs.eval()that I am missing? I set it to False so that someone can look at the failing logs and maybe figure it out. I am also pasting the complete unhelpful traceback I get in PyCharm below. Possibly related to InverseGamma logcdf method fails with invalid parameters when array is used #4340 which was "hacked" away in Increase unittestcheck_logcdfcoverage and fix issues with some distribution methods #4393.test_wald_scipyhas a 4 year old@pytest.mark.xfailreferring to a rather old scipy issue. It is passing locally on my machine, so maybe it can be removed? I commented it out to see if it passes here on Github. https://github.com/pymc-devs/pymc3/blame/master/pymc3/tests/test_distributions.py#L810 Update: Seems to be a float32 specific xfail. Added the specific condition.Gamma / InverseGamma traceback
Follow-up question: Do you think it would be helpful to add these new checks to
pymc3_matches_scipy(the equivalent test for logp methods)?