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

Fix get_vector_length incorrectly returning for shared variable without static shape #1295

Merged
merged 2 commits into from
Mar 24, 2025

Conversation

Abhinav-Khot
Copy link
Contributor

@Abhinav-Khot Abhinav-Khot commented Mar 14, 2025

Description

Fixed get_vector_length returning a value when called with a shared variable which did not have static shape. This was done by removing a dispatch implemented in tensor/shared_var.py. After the changes, a ValueError is raised when a shared variable without static shape is passed.

Related Issue

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

📚 Documentation preview 📚: https://pytensor--1295.org.readthedocs.build/en/1295/

@Abhinav-Khot
Copy link
Contributor Author

Abhinav-Khot commented Mar 14, 2025

There seem to be some tests that ensure the earlier behavior, should we deprecate them?

@ricardoV94
Copy link
Member

ricardoV94 commented Mar 18, 2025

There seem to be some tests that ensure the earlier behavior, should we deprecate them?

Keep them but use shared variables with static shape so they still work

Do add a new explicit test that it works with static shape and fails without, separately, unless one of the modified tests is basically that

Copy link

codecov bot commented Mar 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.00%. Comparing base (2a7f3e1) to head (13da552).
Report is 41 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1295   +/-   ##
=======================================
  Coverage   81.99%   82.00%           
=======================================
  Files         188      188           
  Lines       48553    48474   -79     
  Branches     8673     8665    -8     
=======================================
- Hits        39812    39751   -61     
+ Misses       6579     6575    -4     
+ Partials     2162     2148   -14     
Files with missing lines Coverage Δ
pytensor/tensor/sharedvar.py 90.47% <ø> (-0.83%) ⬇️

... and 28 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Abhinav-Khot
Copy link
Contributor Author

There seem to be some tests that ensure the earlier behavior, should we deprecate them?

Keep them but use shared variables with static shape so they still work

Do add a new explicit test that it works with static shape and fails without, separately, unless one of the modified tests is basically that

I have added this, could you please review?

@ricardoV94 ricardoV94 changed the title Fix get_vector_length incorrectly returning for shared variable without static shape Fix get_vector_length incorrectly returning for shared variable without static shape Mar 24, 2025
@ricardoV94 ricardoV94 merged commit 2e9d502 into pymc-devs:main Mar 24, 2025
73 checks passed
@ricardoV94
Copy link
Member

Thanks @Abhinav-Khot

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.

get_vector_length incorrectly returns for shared variable without static shape
2 participants