-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
a6fe6eb
to
cbc3198
Compare
Okay the weird stuff with the KroneckerNormal is that it had an invalid signature, since it can receive a variable number of covariances! |
cbc3198
to
167f0a0
Compare
@OriolAbril the RTD seems to be erroring out on one of the NBs in a weird way. I can run it locally just fine |
167f0a0
to
e8f0713
Compare
There was a problem hiding this 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 SymbolicRandomVariable
s. Is it related to being more explicit about the produced rv_op
s 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 |
There was a problem hiding this 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?
See reply above #7370 (comment) |
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 |
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 |
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 |
Something is definitely off with remote RTD. Now another notebook is failing because it's trying to pass �[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 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ 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
|
97b355d
to
e8f0713
Compare
Since RTD was failing elsewhere (see #7384) and seems unrelated to this PR I reverted the patch commit that added the |
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.