Skip to content

Fix compute_test_value error when creating observed variables #6982

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 7 commits into from
Nov 2, 2023

Conversation

vandalt
Copy link
Contributor

@vandalt vandalt commented Nov 1, 2023

What is this PR about?
This fixes #6981. The issue was about observed variables causing a TypeError when a model is created with pytensor.config.compute_test_value != "off" It adds a small test that reproduces the issue and replaces rv_var.shape by rv_var.type.shape in the np.broadcast_to() call.

Let me know if I should move the test elsewhere or if something else is needed!

Thanks!

Checklist

Bugfixes


📚 Documentation preview 📚: https://pymc--6982.org.readthedocs.build/en/6982/

…ed RV in model creation (Fix pymc-devs#6981)

- Issue: pymc-devs#6981
- This fix only affects cases with `pytensor.config.compute_test_value!="off"`. Code is not executed otherwise.
Copy link

welcome bot commented Nov 1, 2023

Thank You Banner
💖 Thanks for opening this pull request! 💖 The PyMC community really appreciates your time and effort to contribute to the project. Please make sure you have read our Contributing Guidelines and filled in our pull request template to the best of your ability.

Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

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

Thanks for opening a PR.

This is related to some functionality that is not much needed in PyMC anymore and we may remove this PyTensor flag in the near future. Anyway, I've left some comments below

Should always have the right shape when not None
There are no cases where the `test_value` should be None, so the code inside that block should not ever be used.
Copy link

codecov bot commented Nov 1, 2023

Codecov Report

Merging #6982 (c3b7498) into main (58217cc) will increase coverage by 0.40%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6982      +/-   ##
==========================================
+ Coverage   91.70%   92.10%   +0.40%     
==========================================
  Files         100      100              
  Lines       16899    16894       -5     
==========================================
+ Hits        15497    15560      +63     
+ Misses       1402     1334      -68     
Files Coverage Δ
pymc/model/core.py 91.75% <ø> (+0.45%) ⬆️

... and 4 files with indirect coverage changes

@ricardoV94 ricardoV94 added the bug label Nov 1, 2023
@ricardoV94 ricardoV94 changed the title Fix TypeError caused by numpy broadcast of observed RV with pytensor.config.compute_test_value != off Fix compute_test_value error when creating observed variables Nov 2, 2023
@ricardoV94
Copy link
Member

ricardoV94 commented Nov 2, 2023

I pushed a commit that simplifies the test, after confirming it still failed in the main branch.

Thanks for the PR. This is the best kind of bug fix :D

@ricardoV94 ricardoV94 merged commit 714b4a0 into pymc-devs:main Nov 2, 2023
Copy link

welcome bot commented Nov 2, 2023

Congratulations Banner
Congrats on merging your first pull request! 🎉 We here at PyMC are proud of you! 💖 Thank you so much for your contribution 🎁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: TypeError: Variables do not support boolean operations with compute_test_value = "raise" for observed random variable
2 participants