Skip to content
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

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Feb 14, 2023

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 let Model 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

@ricardoV94 ricardoV94 added maintenance major Include in major changes release notes section labels Feb 14, 2023
@codecov
Copy link

codecov bot commented Feb 14, 2023

Codecov Report

Merging #6523 (ecba5bb) into main (14a10b7) will increase coverage by 0.00%.
The diff coverage is 98.55%.

Additional details and impacted files

Impacted file tree graph

@@           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     
Impacted Files Coverage Δ
pymc/util.py 80.00% <ø> (ø)
pymc/model.py 89.79% <88.88%> (-0.11%) ⬇️
pymc/logprob/joint_logprob.py 98.88% <100.00%> (-0.35%) ⬇️
pymc/variational/minibatch_rv.py 100.00% <100.00%> (ø)
pymc/variational/opvi.py 87.48% <100.00%> (ø)

@ricardoV94 ricardoV94 force-pushed the remove_totalsize_mapping branch from 05e7547 to bbcf613 Compare February 14, 2023 16:29
@ricardoV94 ricardoV94 force-pushed the remove_totalsize_mapping branch from bbcf613 to fc8cf39 Compare February 28, 2023 10:58
@ricardoV94 ricardoV94 changed the title Replace rvs_to_total_sizes mapping by RescaledRandomVariables Replace rvs_to_total_sizes mapping by MinibatchRandomVariables Feb 28, 2023
Copy link
Member

@ferrine ferrine left a 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

pymc/variational/minibatch_rv.py Outdated Show resolved Hide resolved


@_logprob.register(MinibatchRandomVariable)
def minibatch_rv_logprob(op, values, *inputs, **kwargs):
Copy link
Member

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

@ricardoV94 ricardoV94 force-pushed the remove_totalsize_mapping branch from fc8cf39 to ecba5bb Compare February 28, 2023 12:29
@ricardoV94 ricardoV94 merged commit 1ca1c94 into pymc-devs:main Feb 28, 2023
@ricardoV94 ricardoV94 deleted the remove_totalsize_mapping branch June 6, 2023 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance major Include in major changes release notes section
Projects
None yet
2 participants