Skip to content

Small compatibility fix for master of Theano-PyMc #4293

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

Merged
merged 3 commits into from
Dec 10, 2020

Conversation

semohr
Copy link
Contributor

@semohr semohr commented Dec 4, 2020

  • Import of pymc3 failed with the most recent master branch of Theano-PyMC
  • I could nail down the problem to this commit on the Theano-PyMC GitHub
  • Changed the __init__.py to use the new config style

@twiecki
Copy link
Member

twiecki commented Dec 4, 2020

We probably need a new Theano release and pin.

@twiecki
Copy link
Member

twiecki commented Dec 4, 2020

CC @brandonwillard @dfm

@MarcoGorelli
Copy link
Contributor

Maybe after this is fixed we also need a theano-pymc-compatibility CI job which installs the latest commit to master (similarly to the arviz-compatibility one)

@brandonwillard
Copy link
Contributor

brandonwillard commented Dec 4, 2020

There is already an open Theano-PyMC compatibility update PR that's waiting for the next release of Theano-PyMC (the PR title is too specific to one of the updates, though). All these updates are better off in a single PR corresponding to a single new release of Theano-PyMC.

The changes addressed in this PR were merged just last night, so they haven't been added to the existing PR, and the version of Theano-PyMC used by PyMC3 should already be pinned in this repo (from a commit merged a while back). In other words, the relevant Theano-PyMC changes aren't an active issue, since one would need to install an explicitly unsupported version of Theano-PyMC for them to appear.

@semohr semohr closed this Dec 4, 2020
@semohr semohr reopened this Dec 4, 2020
@semohr
Copy link
Contributor Author

semohr commented Dec 4, 2020

Sorry if I wasted your time here a bit. I just tried to get the new jax sampler working, I noticed this problem and found a easy fix for it.

Thought I should share this, since maybe someone else could have the same problem.

I'm not too sure how you want to handle such small fixes.
Feel free to delete this PR.

Best Sebastian

@brandonwillard
Copy link
Contributor

brandonwillard commented Dec 4, 2020

If you want to take on these recent updates, you can pull in the changes from the existing PR and make this one the cumulative update PR. There shouldn't be much in that PR, so you could probably just look at the diffs and apply them manually.

We simply need those changes in a single PR, because we can't merge them one at a time—at least not without temporarily putting the repo in a truly broken state.

Otherwise, did you install a development version of PyMC3 (e.g. master) and it pulled in the most recent Theano-PyMC? If that's the case, we should probably find out why and fix it, because it implies that our pinned version is being ignored/circumvented.

@semohr
Copy link
Contributor Author

semohr commented Dec 4, 2020

I totally unterstand, will look into that.

Yes I installed the master branch. Tried the jax branch beforehand but that one also installed the master theano-pymc branch.

@brandonwillard
Copy link
Contributor

brandonwillard commented Dec 4, 2020

Tried the jax branch beforehand but that one also installed the master theano-pymc branch.

Ah, that's probably it; if you're referring to the pymc3jax branch, it might need to be rebased onto master in order to get the commit that pinned Theano-PyMC (and some previous compatibility updates, as well).

@twiecki, wasn't the pymc3jax merged? If so, we might want to delete it; otherwise, people could accidently use it and run into problems.

@brandonwillard
Copy link
Contributor

The updates in this PR will also close aesara-devs/aesara#216.

@codecov
Copy link

codecov bot commented Dec 8, 2020

Codecov Report

Merging #4293 (8ac3e3a) into master (0431292) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4293      +/-   ##
==========================================
- Coverage   87.56%   87.56%   -0.01%     
==========================================
  Files          88       88              
  Lines       14270    14270              
==========================================
- Hits        12496    12495       -1     
- Misses       1774     1775       +1     
Impacted Files Coverage Δ
pymc3/distributions/multivariate.py 83.33% <100.00%> (ø)
pymc3/math.py 68.47% <100.00%> (ø)
pymc3/ode/ode.py 93.81% <100.00%> (ø)
pymc3/theanof.py 89.61% <0.00%> (-0.44%) ⬇️

@brandonwillard
Copy link
Contributor

This won't pass until conda-forge/theano-pymc-feedstock#8 is merged and theano-pymc==1.0.12 is made available via conda-forge.

@twiecki
Copy link
Member

twiecki commented Dec 9, 2020

Merged now.

@twiecki twiecki merged commit 4fd56fd into pymc-devs:master Dec 10, 2020
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.

4 participants