Skip to content

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Jul 2, 2025

This showed up in the PyMC tests. There were some mistakes with the alignment of a, b batch and core dimensions.

The tests are failing for CholeskySolve, it seems to come back with flipped signs. Is this expected?


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

@ricardoV94 ricardoV94 requested a review from jessegrabowski July 2, 2025 16:26
@ricardoV94 ricardoV94 added bug Something isn't working graph rewriting linalg Linear algebra labels Jul 2, 2025
Comment on lines +1049 to +1051
if core_op.b_ndim == 1:
# Convert b to a column matrix
b = b[..., None]
Copy link
Member Author

@ricardoV94 ricardoV94 Jul 2, 2025

Choose a reason for hiding this comment

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

If a had shape (1, 1, 1) and b had shape (5, 1) and b_ndim=1 (meaning 5 is a batch dimension for b), we were returning something with the wrong shape as it became (1, 5, 1), after broadcasting with A, and then (1, 5) after squeezing below

Copy link
Member

@jessegrabowski jessegrabowski left a comment

Choose a reason for hiding this comment

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

oops :D

@jessegrabowski
Copy link
Member

I approved too quickly. Regarding the flipped signs, I need to look more closely. There might be sign ambiguity due to the squared square root, or there might be a bug.

@ricardoV94
Copy link
Member Author

Tests are not passing so no accident of merging

@jessegrabowski
Copy link
Member

I think the problem might be the test. Numerical cholesky routines always compute such that the main diagonal is positive, which resolves any sign ambiguity in cho_solve. Here we're drawing the test value from a random normal, so there is sign ambiguity in the test.

I think if you square a_val, at least in the cho_solve case, tests will pass.

@ricardoV94 ricardoV94 force-pushed the fix_bug_solve_scalar_rewrite branch from 13d8124 to e8a0b0b Compare July 2, 2025 16:53
@ricardoV94
Copy link
Member Author

Squaring worked like a charm

Copy link

codecov bot commented Jul 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.03%. Comparing base (2ab60b3) to head (e8a0b0b).
⚠️ Report is 118 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1516      +/-   ##
==========================================
+ Coverage   81.98%   82.03%   +0.05%     
==========================================
  Files         231      231              
  Lines       52274    52350      +76     
  Branches     9206     9215       +9     
==========================================
+ Hits        42856    42946      +90     
+ Misses       7106     7095      -11     
+ Partials     2312     2309       -3     
Files with missing lines Coverage Δ
pytensor/tensor/rewriting/linalg.py 92.08% <100.00%> (+0.03%) ⬆️

... and 11 files with indirect coverage changes

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

@ricardoV94 ricardoV94 merged commit 815671d into pymc-devs:main Jul 2, 2025
74 checks passed
@ricardoV94 ricardoV94 changed the title Fix shape errors in scalar_solve_to_division Fix shape errors in scalar_solve_to_division rewrite Jul 2, 2025
@ricardoV94 ricardoV94 deleted the fix_bug_solve_scalar_rewrite branch July 2, 2025 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working graph rewriting linalg Linear algebra

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants