Skip to content

Upgrade Aesara version pin and unpin SciPy upper limit #5474

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 9 commits into from
Feb 27, 2022

Conversation

michaelosthege
Copy link
Member

I'm opening the PR so the CI tells me which tests break.

Closes #5446
Closes #5448

@codecov
Copy link

codecov bot commented Feb 15, 2022

Codecov Report

Merging #5474 (bdc6e85) into main (da2030e) will increase coverage by 5.77%.
The diff coverage is 95.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5474      +/-   ##
==========================================
+ Coverage   81.46%   87.23%   +5.77%     
==========================================
  Files          81       81              
  Lines       14213    14247      +34     
==========================================
+ Hits        11579    12429     +850     
+ Misses       2634     1818     -816     
Impacted Files Coverage Δ
pymc/distributions/distribution.py 90.80% <71.42%> (-0.64%) ⬇️
pymc/aesaraf.py 91.18% <100.00%> (+1.00%) ⬆️
pymc/distributions/censored.py 95.00% <100.00%> (+1.77%) ⬆️
pymc/distributions/multivariate.py 92.16% <100.00%> (+0.42%) ⬆️
pymc/distributions/shape_utils.py 97.00% <100.00%> (+0.26%) ⬆️
pymc/util.py 77.50% <100.00%> (+1.87%) ⬆️
pymc/sampling_jax.py 96.89% <0.00%> (-1.41%) ⬇️
pymc/bart/pgbart.py 94.92% <0.00%> (-0.32%) ⬇️
pymc/backends/arviz.py 90.28% <0.00%> (ø)
pymc/distributions/continuous.py 97.23% <0.00%> (ø)
... and 19 more

Copy link
Member Author

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

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

I ran only the tests that were red, so maybe I broke something. (We'll see.)

But at least the following tests are still failing, because the RV implementations need to be fixed:

FAILED pymc/tests/test_distributions.py::TestMatchesScipy::test_dirichlet_vectorized[2-a2] - ValueError: input operand has more dimensions than allowed by the axis remapping
FAILED pymc/tests/test_distributions.py::TestMatchesScipy::test_dirichlet_vectorized[size1-a2] - ValueError: shape mismatch: objects cannot be broadcast to a single shape
FAILED pymc/tests/test_distributions.py::TestMatchesScipy::test_dirichlet_vectorized[size2-a1] - ValueError: shape mismatch: objects cannot be broadcast to a single shape
FAILED pymc/tests/test_distributions.py::TestMatchesScipy::test_dirichlet_vectorized[size2-a2] - ValueError: shape mismatch: objects cannot be broadcast to a single shape
FAILED pymc/tests/test_distributions_moments.py::test_dirichlet_moment[a2-7-expected2] - TypeError: Cannot convert Type TensorType(float64, (None, None, None)) (of Variable Elemwise{true_div,no_inplace}.0) ...
FAILED pymc/tests/test_distributions_moments.py::test_dirichlet_moment[a3-size3-expected3] - TypeError: Cannot convert Type TensorType(float64, (None, None, None, None)) (of Variable Elemwise{true_div,no_in...
FAILED pymc/tests/test_distributions_moments.py::test_mv_normal_moment[mu7-cov7-size7-expected7] - ValueError: could not broadcast input array from shape (2,2) into shape (4,5,2)
FAILED pymc/tests/test_distributions_moments.py::test_mvstudentt_moment[2-mu5-cov5-2-expected5] - assert (2, 3) == (2, 2, 3)
FAILED pymc/tests/test_distributions_moments.py::test_mvstudentt_moment[2-mu6-cov6-size6-expected6] - ValueError: operands could not be broadcast together with shapes (2,5,2,3) (2,5,2,1,1)
FAILED pymc/tests/test_distributions_moments.py::test_multinomial_moment[p6-n6-2-expected6] - TypeError: Cannot convert Type TensorType(int64, (None, None, None)) (of Variable Elemwise{Cast{int64}}.0) into ...
FAILED pymc/tests/test_distributions_random.py::TestDirichletMultinomial_1D_n_2D_a::test_distribution - assert (1, 2, 4) == (1, 4)

For MvNormal and MvStudentT I think this is just the rng_fn (and get_moment).


I gotta jump now, so anybody please feel free to take over.

rv_out = change_rv_size_fn(rv_var=rv_out, new_size=shape[:-1])
# Into added dims 👇 plus implied dims 👇 but excluding support 👇 dims.
new_size = (*shape[:-1], *tuple(rv_out.shape[:ndim_actual - ndim_supp]))
rv_out = change_rv_size_fn(rv_var=rv_out, new_size=new_size)
Copy link
Member Author

Choose a reason for hiding this comment

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

The else: below may no longer be relevant, but we should first add type hints to the signature, because when I tried to un-indent the ShapeError it turned out that some of the int parameters are actually Optional[int].

Copy link
Member Author

@michaelosthege michaelosthege Feb 22, 2022

Choose a reason for hiding this comment

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

@ricardoV94 it looks like this whole block in shape_utils.py is no longer covered by any tests. It is covered on main (line 661).
Can we remove it, or is there an edge case that could still trigger these branches?

grafik

Copy link
Member

@ricardoV94 ricardoV94 Feb 22, 2022

Choose a reason for hiding this comment

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

is there an edge case that could still trigger these branches?

None that I know of

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like I messed up the ShapeWarning. I'll take a look at it in the morning.

@OriolAbril
Copy link
Member

OriolAbril commented Feb 15, 2022

You should also check https://pymc--5474.org.readthedocs.build/en/5474/learn/examples/pymc_overview.html#model-specification to make sure not only the installation finishes but that it is actually working and pymc can be imported and used (same url will work as new commits come in)

@ricardoV94 ricardoV94 force-pushed the upgrade-aesara branch 4 times, most recently from c50e0fa to 4767081 Compare February 22, 2022 11:04
mv_samples = multivariate_normal.rng_fn(rng=rng, mean=np.zeros_like(mu), cov=cov, size=size)

size = tuple(size or ())
# Take chi2 draws and add an axis of length 1 to the right for correct broadcasting below
chi2_samples = np.sqrt(rng.chisquare(nu, size=size) / nu)[..., None]
Copy link
Member

Choose a reason for hiding this comment

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

@Sayam753 can you have a second look if the new Multivariate rng_fn looks correct?

@ricardoV94 ricardoV94 force-pushed the upgrade-aesara branch 3 times, most recently from 5dd186e to 211e547 Compare February 22, 2022 11:38
ricardoV94 and others added 3 commits February 22, 2022 13:04
These tests were introduced between pre-existing Dirichlet/Multinomial/DirichletMultionmial tests that belong to conceptually related distributions
@ricardoV94
Copy link
Member

ricardoV94 commented Feb 22, 2022

Tests are passing, and I doubled check the following suspicious (initially) non-failing distributions:

  • MatrixNormal
  • KroneckerNormal
  • CAR

@ricardoV94 ricardoV94 marked this pull request as ready for review February 22, 2022 15:12
Copy link
Member Author

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

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

With the last commit mypy -p pymc has zero complaints about shape_utils.py :)

@michaelosthege michaelosthege force-pushed the upgrade-aesara branch 2 times, most recently from 4c2c095 to 2fde55e Compare February 23, 2022 20:42
@michaelosthege michaelosthege merged commit 2c6abf6 into pymc-devs:main Feb 27, 2022
@michaelosthege michaelosthege deleted the upgrade-aesara branch February 27, 2022 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants