-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Replace rvs_to_total_sizes mapping by MinibatchRandomVariables #6523
Replace rvs_to_total_sizes mapping by MinibatchRandomVariables #6523
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6523 +/- ##
=======================================
Coverage 91.91% 91.92%
=======================================
Files 89 90 +1
Lines 14936 14955 +19
=======================================
+ Hits 13729 13747 +18
- Misses 1207 1208 +1
|
05e7547
to
bbcf613
Compare
bbcf613
to
fc8cf39
Compare
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.
I like the proposed design change
|
||
|
||
@_logprob.register(MinibatchRandomVariable) | ||
def minibatch_rv_logprob(op, values, *inputs, **kwargs): |
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.
This is much more intuitive now
fc8cf39
to
ecba5bb
Compare
This PR refactors the way rescaling of minibatched variables is achieved, so as to not require keeping any meta-information at the model level.
Ideally, the API would follow that of other RV factories with something like
pm.MinibatchRV(dist=dist, total_size=...)
so that we don't have to add any extra complexity at the Model level, but for the sake of backwards compatibility we letModel
do this for now.I removed support for scaling of non-observed RVs even though it would be equally easy to support them. This is a breaking change
Closes #6061
Closes #6332