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

Upgrades to PyMC compatibility #61

Merged
merged 7 commits into from
Dec 14, 2022
Merged

Upgrades to PyMC compatibility #61

merged 7 commits into from
Dec 14, 2022

Conversation

michaelosthege
Copy link
Member

@michaelosthege michaelosthege commented Dec 13, 2022

Major changes

  • Dropping pymc3 compatibility
  • Removed PyGMO integration (Closes pygmo deprecation? #47)
  • Testing with pymc==4.4.0
  • Testing with pymc==5.0.0

Maintenance

  • Removed unused GitLab CI definition
  • Bumped versions of Python and GitHub actions
  • Re-ran some of the documentation notebooks

* Removed PyGMO
* Switched from Student-t to Normal-distributed noise

Part of #47
We didn't use it internally, and it is not yet available for Python 3.10.

Closes #47
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2022

Codecov Report

Merging #61 (035135f) into master (37bfb11) will decrease coverage by 0.25%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
- Coverage   97.49%   97.23%   -0.26%     
==========================================
  Files           6        7       +1     
  Lines         919      905      -14     
==========================================
- Hits          896      880      -16     
- Misses         23       25       +2     
Impacted Files Coverage Δ
calibr8/contrib/noise.py 100.00% <ø> (ø)
calibr8/utils.py 91.79% <80.95%> (-1.62%) ⬇️
calibr8/__init__.py 100.00% <100.00%> (ø)
calibr8/core.py 99.73% <100.00%> (+<0.01%) ⬆️
calibr8/optimization.py 89.74% <100.00%> (-2.20%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

This also refactors the way how log-likelihood graphs are created,
because the API for that changed between v4 and v5.

Also re-runs some example notebooks.
@michaelosthege michaelosthege marked this pull request as ready for review December 14, 2022 00:35
@michaelosthege
Copy link
Member Author

omg, finally it's green 😌

Copy link
Member

@lhelleckes lhelleckes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like A LOT of work. I haven't checked the notebooks yet since this is nasty in the browser. Will do so locally tomorrow.

calibr8/core.py Outdated
if pm.__version__[0] == "4":
return pm.joint_logp(rv, y, sum=True)
elif int(pm.__version__[0]) >= 5:
return pm.logprob.joint_logprob({rv: y_tensor}, sum=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in PyMC v5 this is only working with tensor inputs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the previous pm.joint_logp was a wrapper that apparently did this automatically..

Copy link
Member

@lhelleckes lhelleckes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also reviewed the notebooks. The deprecation warning are not so nice, but since you have a watermark showing the PyMC version, this should be fine.

if unexpected_rv_nodes:
raise ValueError(
f"Random variables detected in the logp graph: {unexpected_rv_nodes}.\n"
"This can happen when DensityDist logp or Interval transform functions "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure anybody except you or maybe me could fix this in case of an error, but I guess that's ok.

@lhelleckes lhelleckes merged commit 1d220a6 into master Dec 14, 2022
@michaelosthege michaelosthege deleted the pymc5-compat branch December 14, 2022 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pygmo deprecation?
3 participants