pymc-devs / pymc

Bayesian Modeling and Probabilistic Programming in Python
https://docs.pymc.io/
Other
8.47k stars 1.97k forks source link

Bump PyTensor dependency (which changes signature of RandomVariables) #7370

Closed ricardoV94 closed 1 week ago

ricardoV94 commented 2 weeks ago

See

The new signature functionality and explicit expand_dims revealed several inconsistencies in existing RV implementations, that were fixed and tested in commits before the "Bump PyTensor" one.

review-notebook-app[bot] commented 2 weeks ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

ricardoV94 commented 1 week ago

Okay the weird stuff with the KroneckerNormal is that it had an invalid signature, since it can receive a variable number of covariances!

ricardoV94 commented 1 week ago

@OriolAbril the RTD seems to be erroring out on one of the NBs in a weird way. I can run it locally just fine

ricardoV94 commented 1 week ago

I understand that a major change is the use of NumPy-style signatures, no longer needing ndim_supp and ndims_params. What purpose does extended_signature serve over signature? It seems to (only?) be applicable for SymbolicRandomVariables. Is it related to being more explicit about the produced rv_ops induced by symbolic distributions?

The reason for the extended signature is to provide information about the position of rng inputs/outputs (there could be more than one or none) and the size argument, which may or not exist for SymbolicRandomVariables. This is not a concern for classical RandomVariables that always have exactly one rng and one size argument as the first two inputs and the next rng state as the first output, so their extended signature is always f"[rng],[size],{input_signature}->[rng],{output_signature}". For SymbolicRandomVariable it could look very different, so we have to define it.

ricardoV94 commented 1 week ago

skim-review

When is extended_signature needed?

See reply above https://github.com/pymc-devs/pymc/pull/7370#issuecomment-2183300817

ricardoV94 commented 1 week ago

Okay so the only blocker is RTD failing to run one of the core notebooks. Could this be because I changed the kernel from python3 to pymc (which is the name of my local kernel)?

Can't think of any other reason @OriolAbril

OriolAbril commented 1 week ago

Only the python3 kernel is available, and sphinx is configured to execute all notebooks with it independently of what kernel is listed in the notebook itself.

I'll try to run it locally on a clean env and see what happens. The versions on the rtd log do look right

OriolAbril commented 1 week ago

It worked locally, no idea what is happening, the installation steps should be the same I did, and the builds of the conda-forge packages installed are the same in the rtd logs and in my computer. Maybe add a tag to that cell and see if it fixes itself later on? https://myst-nb.readthedocs.io/en/latest/computation/execute.html#raise-errors-in-code-cells or a try/except block catching the error

ricardoV94 commented 1 week ago

Something is definitely off with remote RTD. Now another notebook is failing because it's trying to pass draws to sample_prior_predictive which was changed in https://github.com/pymc-devs/pymc/pull/7366

TypeError: sample_prior_predictive() got an unexpected keyword argument 'draws'

In https://readthedocs.org/projects/pymc/builds/24789686/

Although that was changed in #7366, the preview of the respective core notebook still shows the old code with samples instead of draws: https://pymcio--7366.org.readthedocs.build/projects/docs/en/7366/learn/core_notebooks/posterior_predictive.html

The dev version of the docs however has the updated code: https://www.pymc.io/projects/docs/en/latest/learn/core_notebooks/posterior_predictive.html

I have no idea what's going on?

codecov[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 92.37537% with 26 lines in your changes missing coverage. Please review.

Project coverage is 92.18%. Comparing base (c2c46a7) to head (97b355d). Report is 1 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/pymc-devs/pymc/pull/7370/graphs/tree.svg?width=650&height=150&src=pr&token=JFuXtOJ4Cb&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs)](https://app.codecov.io/gh/pymc-devs/pymc/pull/7370?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs) ```diff @@ Coverage Diff @@ ## main #7370 +/- ## ========================================== - Coverage 92.36% 92.18% -0.18% ========================================== Files 102 102 Lines 17190 17199 +9 ========================================== - Hits 15877 15855 -22 - Misses 1313 1344 +31 ``` | [Files](https://app.codecov.io/gh/pymc-devs/pymc/pull/7370?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs) | Coverage Δ | | |---|---|---| | [pymc/distributions/censored.py](https://app.codecov.io/gh/pymc-devs/pymc/pull/7370?src=pr&el=tree&filepath=pymc%2Fdistributions%2Fcensored.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9kaXN0cmlidXRpb25zL2NlbnNvcmVkLnB5) | `100.00% <100.00%> (ø)` | | | [pymc/distributions/continuous.py](https://app.codecov.io/gh/pymc-devs/pymc/pull/7370?src=pr&el=tree&filepath=pymc%2Fdistributions%2Fcontinuous.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9kaXN0cmlidXRpb25zL2NvbnRpbnVvdXMucHk=) | `97.91% <100.00%> (+<0.01%)` | :arrow_up: | | [pymc/distributions/discrete.py](https://app.codecov.io/gh/pymc-devs/pymc/pull/7370?src=pr&el=tree&filepath=pymc%2Fdistributions%2Fdiscrete.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9kaXN0cmlidXRpb25zL2Rpc2NyZXRlLnB5) | `99.41% <100.00%> (+0.28%)` | :arrow_up: | | [pymc/distributions/mixture.py](https://app.codecov.io/gh/pymc-devs/pymc/pull/7370?src=pr&el=tree&filepath=pymc%2Fdistributions%2Fmixture.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9kaXN0cmlidXRpb25zL21peHR1cmUucHk=) | `95.02% <100.00%> (ø)` | | | [pymc/distributions/shape\_utils.py](https://app.codecov.io/gh/pymc-devs/pymc/pull/7370?src=pr&el=tree&filepath=pymc%2Fdistributions%2Fshape_utils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9kaXN0cmlidXRpb25zL3NoYXBlX3V0aWxzLnB5) | `90.90% <100.00%> (-4.29%)` | :arrow_down: | | [pymc/distributions/simulator.py](https://app.codecov.io/gh/pymc-devs/pymc/pull/7370?src=pr&el=tree&filepath=pymc%2Fdistributions%2Fsimulator.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9kaXN0cmlidXRpb25zL3NpbXVsYXRvci5weQ==) | `84.28% <100.00%> (+0.11%)` | :arrow_up: | | [pymc/distributions/timeseries.py](https://app.codecov.io/gh/pymc-devs/pymc/pull/7370?src=pr&el=tree&filepath=pymc%2Fdistributions%2Ftimeseries.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9kaXN0cmlidXRpb25zL3RpbWVzZXJpZXMucHk=) | `94.77% <100.00%> (ø)` | | | [pymc/distributions/transforms.py](https://app.codecov.io/gh/pymc-devs/pymc/pull/7370?src=pr&el=tree&filepath=pymc%2Fdistributions%2Ftransforms.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9kaXN0cmlidXRpb25zL3RyYW5zZm9ybXMucHk=) | `98.47% <ø> (ø)` | | | [pymc/distributions/truncated.py](https://app.codecov.io/gh/pymc-devs/pymc/pull/7370?src=pr&el=tree&filepath=pymc%2Fdistributions%2Ftruncated.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9kaXN0cmlidXRpb25zL3RydW5jYXRlZC5weQ==) | `99.45% <100.00%> (ø)` | | | [pymc/logprob/order.py](https://app.codecov.io/gh/pymc-devs/pymc/pull/7370?src=pr&el=tree&filepath=pymc%2Flogprob%2Forder.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9sb2dwcm9iL29yZGVyLnB5) | `94.59% <100.00%> (ø)` | | | ... and [10 more](https://app.codecov.io/gh/pymc-devs/pymc/pull/7370?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs) | |
ricardoV94 commented 1 week ago

Since RTD was failing elsewhere (see #7384) and seems unrelated to this PR I reverted the patch commit that added the raises-exception tag to one cell.