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

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

Merged
merged 7 commits into from
Jun 24, 2024

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Jun 17, 2024

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.

@ricardoV94 ricardoV94 added pytensor major Include in major changes release notes section dependencies Pull requests that update a dependency file labels Jun 17, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@ricardoV94 ricardoV94 force-pushed the bump_pytensor branch 22 times, most recently from a6fe6eb to cbc3198 Compare June 21, 2024 11:43
@ricardoV94
Copy link
Member Author

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

@ricardoV94
Copy link
Member Author

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

Copy link
Member

@larryshamalama larryshamalama left a comment

Choose a reason for hiding this comment

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

Hi @ricardoV94, been a bit distant from PyMC lately, but it's always great to see your work

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?

@ricardoV94
Copy link
Member Author

ricardoV94 commented Jun 21, 2024

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.

Copy link
Member

@twiecki twiecki left a comment

Choose a reason for hiding this comment

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

#skim-review

When is extended_signature needed?

@ricardoV94
Copy link
Member Author

#skim-review

When is extended_signature needed?

See reply above #7370 (comment)

@ricardoV94
Copy link
Member Author

ricardoV94 commented Jun 22, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

ricardoV94 commented Jun 24, 2024

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 #7366

�[0;31mTypeError�[0m: 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?

@ricardoV94 ricardoV94 mentioned this pull request Jun 24, 2024
Copy link

codecov bot commented Jun 24, 2024

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

@@            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 Coverage Δ
pymc/distributions/censored.py 100.00% <100.00%> (ø)
pymc/distributions/continuous.py 97.91% <100.00%> (+<0.01%) ⬆️
pymc/distributions/discrete.py 99.41% <100.00%> (+0.28%) ⬆️
pymc/distributions/mixture.py 95.02% <100.00%> (ø)
pymc/distributions/shape_utils.py 90.90% <100.00%> (-4.29%) ⬇️
pymc/distributions/simulator.py 84.28% <100.00%> (+0.11%) ⬆️
pymc/distributions/timeseries.py 94.77% <100.00%> (ø)
pymc/distributions/transforms.py 98.47% <ø> (ø)
pymc/distributions/truncated.py 99.45% <100.00%> (ø)
pymc/logprob/order.py 94.59% <100.00%> (ø)
... and 10 more

@ricardoV94
Copy link
Member Author

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.

@ricardoV94 ricardoV94 merged commit e71d1cb into pymc-devs:main Jun 24, 2024
36 of 37 checks passed
@ricardoV94 ricardoV94 deleted the bump_pytensor branch June 24, 2024 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file major Include in major changes release notes section pytensor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants