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

Update PyTensor dependency and fix bugs in inferred mixture logprob #6397

Merged
merged 5 commits into from
Dec 15, 2022

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Dec 14, 2022

Will need to wait for the conda-forge release

@codecov
Copy link

codecov bot commented Dec 14, 2022

Codecov Report

Merging #6397 (b54b2d8) into main (f96594b) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6397      +/-   ##
==========================================
+ Coverage   94.79%   94.81%   +0.01%     
==========================================
  Files         148      148              
  Lines       27488    27559      +71     
==========================================
+ Hits        26058    26129      +71     
  Misses       1430     1430              
Impacted Files Coverage Δ
pymc/pytensorf.py 91.72% <ø> (+0.25%) ⬆️
pymc/distributions/distribution.py 97.46% <100.00%> (+<0.01%) ⬆️
pymc/distributions/timeseries.py 94.47% <100.00%> (+0.01%) ⬆️
pymc/logprob/mixture.py 100.00% <100.00%> (ø)
pymc/logprob/transforms.py 97.51% <100.00%> (-0.24%) ⬇️
pymc/sampling/jax.py 98.20% <100.00%> (+<0.01%) ⬆️
pymc/smc/kernels.py 97.41% <100.00%> (ø)
pymc/tests/distributions/test_shape_utils.py 99.76% <100.00%> (ø)
pymc/tests/logprob/test_mixture.py 100.00% <100.00%> (ø)
... and 5 more

@ricardoV94 ricardoV94 force-pushed the update_pytensor_dependency branch 3 times, most recently from d776233 to a051e58 Compare December 14, 2022 16:40
@ricardoV94 ricardoV94 force-pushed the update_pytensor_dependency branch from a051e58 to 31080c6 Compare December 15, 2022 11:22
It did not account for the extra dimension added by the stacking operation, resulting in a logp call to an expanded RV with the original unexpanded value
Now that Dimshuffle lift broadcasts both the parameters and the size, the AdvancedIndexing logprob fails most of the times, even though these are valid graphs.

All but one of the failing cases can be helped by introducing the local_rv_size_lift rewrite.
@ricardoV94 ricardoV94 force-pushed the update_pytensor_dependency branch from 31080c6 to b54b2d8 Compare December 15, 2022 11:26
@ricardoV94 ricardoV94 added the bug label Dec 15, 2022
@ricardoV94 ricardoV94 changed the title Update PyTensor dependency Update PyTensor dependency and fix bugs in inferred mixture logprob Dec 15, 2022
@ricardoV94
Copy link
Member Author

I had to patch quite some things with the inferred mixture logprob. Most of these cases won't be supported after #6369 but we haven't merged that yet

@twiecki
Copy link
Member

twiecki commented Dec 15, 2022

Why don't we?

@ricardoV94
Copy link
Member Author

Why don't we?

Merge? The fixes done here are actually more correct

@ricardoV94 ricardoV94 merged commit 7f3c032 into pymc-devs:main Dec 15, 2022
@ricardoV94 ricardoV94 deleted the update_pytensor_dependency branch April 5, 2023 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants