Automatically derive icdf for negative power transforms #7956
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7956 +/- ##
=======================================
Coverage 91.47% 91.48%
=======================================
Files 116 116
Lines 18947 18935 -12
=======================================
- Hits 17332 17322 -10
+ Misses 1615 1613 -2
🚀 New features to boost your workflow:
|
|
Tests are failing because you can't pass rng to the new signatures |
larryshamalama
left a comment
larryshamalama
left a comment
There was a problem hiding this comment.
Hi @ricardoV94 , great work as always :) I haven't followed much of recent codebase progress, as per @jessegrabowski 's comment other changes seem necessary w.r.t. test failures. I just added minor questions that came to mind as per usual as I read this PR
| msg="alpha > 0, beta > 0", | ||
| ) | ||
|
|
||
| def icdf(value, alpha, beta): |
There was a problem hiding this comment.
I don't visualize the math on top of my head. I presume this is what test_inverse_gamma_icdf below checks?
| lambda value, alpha, beta: st.invgamma.logcdf(value, alpha, scale=beta), | ||
| ) | ||
|
|
||
| def test_inverse_gamma_icdf(self): |
There was a problem hiding this comment.
Some distributions have all functionality tests (logp, logcdf, icdf) under a single method (test_half_cauchy), whereas other distributions have one method per functionality. Is there any particular reason for this?
There was a problem hiding this comment.
No grand plan. I prefer separate but if a function was already testing multiple for that distribution I will just append to it
Co-authored-by: Ricardo Vieira <28983449+ricardov94@users.noreply.github.com>
Co-authored-by: lucaseckes
ricardoV94
force-pushed
the
inverse_icdfs
branch
from
dfa599c to
eec79ee
Compare
CustomDist didn't allow passing explicit rng. I tweaked to allow it. This will be useful as we transition to more explicit rng handling internally in the future. Otherwise I could just test the RV differently, since this is such a trivial RV |
4 participants
(* non even powers)
This way we get icdf easily for InverseGamma and LogitNormal. I removed the latter in favor of a CustomDist. I didn't do it for InverseGamma because that's a pretty popular distribution and don't want to risk messing with precision
closes #7619
closes #7917
closes #7916