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

Upgrade to aesara=2.8.2 and aeppl=0.0.35 #6059

Merged
merged 7 commits into from
Aug 25, 2022

Conversation

Armavica
Copy link
Member

@Armavica Armavica commented Aug 23, 2022

What is this PR about?
Adapting to the new aesara API, fixes #6053.

Checklist

Major / Breaking Changes

  • ...

Bugfixes / New features

  • ...

Docs / Maintenance

  • Upgrade to aesara=2.8.2 and aeppl=0.0.35

@twiecki
Copy link
Member

twiecki commented Aug 23, 2022

Thanks for taking this on @Armavica!

@codecov
Copy link

codecov bot commented Aug 23, 2022

Codecov Report

Merging #6059 (a0bb1db) into main (f7315a3) will increase coverage by 0.50%.
The diff coverage is 98.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6059      +/-   ##
==========================================
+ Coverage   89.06%   89.56%   +0.50%     
==========================================
  Files          72       72              
  Lines       12931    12941      +10     
==========================================
+ Hits        11517    11591      +74     
+ Misses       1414     1350      -64     
Impacted Files Coverage Δ
pymc/distributions/dist_math.py 87.42% <80.00%> (ø)
pymc/aesaraf.py 92.19% <100.00%> (ø)
pymc/distributions/continuous.py 97.50% <100.00%> (+0.02%) ⬆️
pymc/distributions/logprob.py 97.72% <100.00%> (ø)
pymc/distributions/multivariate.py 92.00% <100.00%> (ø)
pymc/distributions/simulator.py 87.50% <100.00%> (ø)
pymc/distributions/timeseries.py 78.64% <100.00%> (ø)
pymc/gp/cov.py 98.09% <100.00%> (ø)
pymc/gp/util.py 96.55% <100.00%> (ø)
pymc/math.py 70.04% <100.00%> (ø)
... and 6 more

@Armavica
Copy link
Member Author

Now the only tests that do not pass are the ones related to the random variable WeibullRV of aesara which might need fixing: aesara-devs/aesara#1141

@Armavica
Copy link
Member Author

So apparently, as discussed in the linked aesara issue, the WeibullRV has been reparameterized to only keep the shape parameter: the scale parameter disappeared.

So I am not sure what to do. The two options that I see are:

  • Keep the current PyMC parameterization and reparameterize for aesara on the fly. Pros: no breaking change and keep a consistent scale parameter among most continuous distributions. Cons: It might be rather hackish code-wise.
  • Propagate the change of parameterization to PyMC, to make Weibull require only one parameter. Pros: one-to-one mapping with aesara. Cons: breaking change, inconsistencies between distributions with a scale parameters and those without.

What are your thoughts?

@ricardoV94
Copy link
Member

We already have a weibull with scale:

class WeibullBetaRV(WeibullRV):

It seems we only need to implement the logp method (you can copy the one that was being used in aeppl until recently)

@Armavica
Copy link
Member Author

Thank you for your help. The tests are now all passing, so I think that the PR is ready for review.

It took me some time to figure out what I had to do for this Weibull distribution, and I still don't understand how everything works… So I hope that I did what you meant, but otherwise I would be happy to redo it differently.

@Armavica Armavica marked this pull request as ready for review August 24, 2022 20:40
@Armavica
Copy link
Member Author

I realized that I forgot to address some deprecations which only raised warnings and not hard errors. So I am going to pass this PR back into draft mode until this is done.

@Armavica Armavica marked this pull request as draft August 24, 2022 21:27
@Armavica Armavica marked this pull request as ready for review August 25, 2022 02:23
@Armavica Armavica requested a review from ricardoV94 August 25, 2022 02:25
Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

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

Awesome work!

@twiecki
Copy link
Member

twiecki commented Aug 25, 2022

Very cool, and already in our new release: https://github.com/pymc-devs/pymc/releases/tag/v4.1.6

@maresb maresb mentioned this pull request Aug 25, 2022
5 tasks
@Armavica Armavica deleted the aesara-rewrite branch August 25, 2022 12:53
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.

Adapt to new aesara API renaming optimize -> rewrite
3 participants