-
Notifications
You must be signed in to change notification settings - Fork 52
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
Support PyMC v5 #309
Support PyMC v5 #309
Conversation
@vandalt — Thanks!! 🚀 It will take me a week or so to go through this in detail, but here are some initial comments. First, can you take a look at the merge conflicts with
Agreed. I think we should leave that as is.
I'll try to get those released ASAP. Might be worth adding a test against the GitHub versions of those in the meantime as well though? What do you think?
This is fine with me as long as
I'm happy to update to 3.8 or 3.9 as the lower bound!
I think this is a big one, but I don't think we should worry about solving it before merging this PR. Instead, we should open a separate issue to track that. |
Thanks!
Yes, I can add that!
I'll try removing and make sure there are not too many warnings when running the tests with
I agree it's a big one, but I think trying a quick update+run for each notebooks (without necessarily fixing everything) would be worthwhile. So far the pRV one helped uncover a problem with passing |
@dfm another point I forgot to raise is that |
Hi @dfm! I finally took time to look at the remaining issues with this PR.
I think the only remaining thing would be to update the Thanks! |
Thanks for all your work on this, @vandalt!! I'll work on getting those other packages released ASAP, but it probably won't be until next week. Can you try to remember to ping me if you don't see any motion by Monday afternoon or Tuesday morning next week? |
@vandalt — I did manage to publish new releases of all of the dependencies yesterday and re-ran the tests. Any guesses for why the PyMC3 tests are failing now? |
Yes sorry, I had forgotten to re-run the tests with PyMC3 locally. Most errors were related to differences in how to access the shape of a variable in PyMC3 vs 5. For the Kipping eccentricity prior, I had added a test for something that was not implemented in the PyMC3 version (truncated distribution with observed eccentricity), so I modified the test to expect the For the |
Great - thanks!! I'll look into debugging the docs build errors because those can be tricky to track down. |
It seems the docs thing was not too tricky (and 100% my fault 😅): I had moved |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
In PyMC 3, we could just used `observed=ecc` with any distribution. In PyMC >= 5, we need to use a `pm.Potential`
Same as kipping13: if eccentricity is "observed" (derived), extra prior must be a `pm.Potential`
Previously, default initval was hardcoded so passing shape without initval caused error. Added failing tests and fixed
…nds in kipping13 This did not work with the original PyMC3 implementation either, but is supported in PyMC 5. This error makes it a bit clearer
for more information, see https://pre-commit.ci
`type.shape` works only in PyMC > 3
The implementation that worked with PyMC v5 did not in PyMC3
Had to remove from the main dependencies because no of the different versions for PyMC3 and PyMC 5
@vandalt — I pushed a few changes to fix some merge conflicts and get the tests passing, but now I think we're good to go here!! I'm going to merge and we can fix issues as they come up in future PRs. Thanks again for all your work getting this over the finish line!! I (and the whole community) really really appreciate everything that you've done here!!! 🚀 |
This builds upon #271, where pretty much all the hard work required to update from PyMC3 had already been done, and implements support of PyMC v5. Most of the changes were replacing
aesara
bypytensor
plus a few API changes.Some things left to address:
joss
directory be updated? I'm guessing the answer is noexoplanet-core
,pymc-ext
andcelerite2
oncepymc >= 5
support is released there. This is required for the tests to pass.utils.docs_setup()
? I left TODO comments to discuss this.pytensor.opt.Assert
(which used to betheano.opt.Assert
) which no longer exists. See comment in the codenox
(and the lowest still getting security updates I think)main
, but probably worth waiting in case it exposes bugs or deprecation warnings.