-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Rename logprob/joint_logp to logprob/basic and move logcdf and icdf functions there
#6599
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rename logprob/joint_logp to logprob/basic and move logcdf and icdf functions there
#6599
Conversation
logprob/joint_logp to logprob/basic and move logcdf and icdf helpers there
logprob/joint_logp to logprob/basic and move logcdf and icdf helpers therelogprob/joint_logp to logprob/basic and move logcdf and icdf functions there
dc42929 to
bf2b1d7
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6599 +/- ##
==========================================
- Coverage 92.03% 92.02% -0.02%
==========================================
Files 93 93
Lines 15746 15750 +4
==========================================
+ Hits 14492 14494 +2
- Misses 1254 1256 +2
|
bf2b1d7 to
4e14f43
Compare
pymc/logprob/basic.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me see if I understand this correctly:
We enter this NonImplementedError in cases where the rv.owner.op has a type that was not registered anywhere with @_logprob.register(...), and so that falls back to the default (non)-implementation in abstract.py.
In these cases, why would the FunctionGraph approach work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It falls back to the intermediate representation (IR) which converts stuff like Exp(Beta) into MeasurableExp(Beta) (or whatever is deemed valid), which itself has a logp method dispatched.
That's basically how inference for non basic RVs is carried out.
7dec389 to
a9ff874
Compare
a9ff874 to
7821ebe
Compare
|
Ready for re-review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think I understand this well enough. I like the clearer hierarchy of functions and the reduction of user-facing logp-related functions!
This PR is a spinoff of #6597, to facilitate review.
Important changes:
logprob/joint_logprobmodule was renamed tologprob/basic.pyIt holds all the user-facing probability constructors
logpno longer calls the heavier factorized_joint_logprob. It immediately dispatches on the IR