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

Simplify measurable transform rewrites #6370

Merged

Conversation

ricardoV94
Copy link
Member

Just some cleanup of the logprob submodule, removing functionality we don't need or are not using in PyMC

Closes #6353

@ricardoV94
Copy link
Member Author

I would like to get this one merged before the group hackathon tomorrow. Will make it easier to review the other more-interesting PRs

@ricardoV94 ricardoV94 force-pushed the simplify_measurable_transform_rewrites branch from 9bd018d to 0d21996 Compare December 5, 2022 11:21
@codecov
Copy link

codecov bot commented Dec 5, 2022

Codecov Report

Merging #6370 (e11b576) into main (5598298) will decrease coverage by 13.99%.
The diff coverage is 77.77%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #6370       +/-   ##
===========================================
- Coverage   94.72%   80.72%   -14.00%     
===========================================
  Files         132      131        -1     
  Lines       26742    26672       -70     
===========================================
- Hits        25331    21531     -3800     
- Misses       1411     5141     +3730     
Impacted Files Coverage Δ
pymc/logprob/cumsum.py 96.87% <ø> (-3.13%) ⬇️
pymc/logprob/mixture.py 36.36% <ø> (-63.64%) ⬇️
pymc/tests/logprob/test_transforms.py 0.00% <0.00%> (-99.31%) ⬇️
pymc/logprob/censoring.py 70.10% <100.00%> (-29.90%) ⬇️
pymc/logprob/rewriting.py 91.17% <100.00%> (-5.89%) ⬇️
pymc/logprob/scan.py 19.57% <100.00%> (-77.78%) ⬇️
pymc/logprob/tensor.py 75.00% <100.00%> (-12.04%) ⬇️
pymc/logprob/transforms.py 82.27% <100.00%> (-17.14%) ⬇️
pymc/tests/logprob/test_utils.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/logprob/test_cumsum.py 0.00% <0.00%> (-100.00%) ⬇️
... and 38 more

These should have been provided as keyword arguments, but they don't seem to matter anyway
This functionality was not being used by PyMC. In theory, it could be used to cache one type of transformed RV Op instead of recreating it for every instance of the same RV + transform, resulting in a faster logp rewrite (?). We can always revisit this idea later, but for now it unnecessarily complicated our codebase.
@ricardoV94 ricardoV94 force-pushed the simplify_measurable_transform_rewrites branch from 0d21996 to e11b576 Compare December 5, 2022 13:21
@ricardoV94 ricardoV94 merged commit ddc6b65 into pymc-devs:main Dec 5, 2022
@ricardoV94 ricardoV94 deleted the simplify_measurable_transform_rewrites branch June 6, 2023 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Order of rewrites in logprob submodule is not respected
2 participants