Replace incomplete_beta with at.betainc and speedup/clean logcdf tests
#4857
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4857 +/- ##
===========================================
+ Coverage 49.24% 61.41% +12.16%
===========================================
Files 86 85 -1
Lines 13787 13771 -16
===========================================
+ Hits 6789 8457 +1668
+ Misses 6998 5314 -1684
|
ricardoV94
force-pushed
the
inc_beta
branch
2 times, most recently
from
7486bf7
to
07d0bf4
Compare
| value: numeric | ||
| Value(s) for which log CDF is calculated. | ||
| value: numeric or np.ndarray or aesara.tensor | ||
| Value(s) for which log CDF is calculated. If the log CDF for multiple | ||
| values are desired the values must be provided in a numpy array or Aesara tensor. |
There was a problem hiding this comment.
Might as well keep this parameter description simple and put all the type-based information in type annotations.
There was a problem hiding this comment.
Yeah, these docstrings could benefit from an overhaul (or be removed altogether). I simply changed them to be equal to those of the remaining distributions.
There was a problem hiding this comment.
Opened an issue here: #4859
| params["value"] = value # for displaying in err_msg | ||
| with aesara.config.change_flags(on_opt_error="raise", mode=Mode("py")): |
There was a problem hiding this comment.
Are we setting on_opt_error="raise" elsewhere? This is a good option to have set globally.
Also, are we sure we want to compile all these tests via C? The Python-based graphs can sometimes be about as quick as the C-compiled graphs + their compilation time.
There was a problem hiding this comment.
Are we setting on_opt_error="raise" elsewhere? This is a good option to have set globally.
I don't think we are. You mean set it up for all the tests?
Also, are we sure we want to compile all these tests via C? The Python-based graphs can sometimes be about as quick as the C-compiled graphs + their compilation time.
I would say it's better to test them with the default back-end, which for the time being is C
Added a test to cover the deprecation warning |
I already added that in my last force-push.
It doesn't use the |
There are Aesara lines in there and those env files are used for the test pipelines.. |
|
Changed the conda-envs |
Also reverts reduced test n_samples due to speed issues
…nverseGamma` Closes pymc-devs#4467
Implement Betainc
This PR deprecates the
incomplete_betamethod indist_math.pyin favor of the newaesara.tensor.betaincwhich wraps thescipy.special.betaincmethod. Supersedes #4736; Closes #4420.This change provides a large increase in speed (and graph compilation) for distributions that made use of the
incomplete_betaand/or it's gradients. In addition, the logcdf of Beta, StudentT, Binomial, NegativeBinomial (and zero inflated variants) can now evaluate multiple values.The downside is the constant warning that the op has no
c-implementationwhenever the logp/logcdf makes use of it. This limitation is tracked in aesara-devs/aesara#267 and aesara-devs/aesara#514Speedup logcdf tests
The logcdf tests were refactored to avoid recreating the logcdf graph for every parameter / value evaluation. This supersedes #4734. The old
incomplete_betawas found to be defective when attempting #4734, and so I decided to merge the two PRs into this one instead.This allows to remove most of the custom
n_sampleslimitations, and makes the test suite run considerably faster.Other changes
HyperGeometrictests caused by LogSumExp of-infreturnsnaninstead of-infaesara-devs/aesara#461 was removed.GammaandInverseGammalogcdf methods was removed (closes Remove hacks to avoid previous aesara gammainc(c) limitations #4467)