Skip to content

Fix bug in gemv c code #1408

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 2 commits into from
May 21, 2025

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented May 20, 2025

Fix bug in handling of row/column matrices in GEMV c_code

Bug was caused by reusing the blas-friendly readjusted strides in the logic to decide whether the call to GEMV should be transposed or not.

Particularly the old +1 in the strides was causing the last error branch to be triggered when the matrix had shape (1, 1). The +1 was supposedly there for the cases of matrix with length 0, but that has it's own branch where the adjusted strides are never used. [That branch is actually useless, and was also removed in this PR]

This bug was introduced in afe934b, before it we were using the unadjusted strides, but that is also suboptimal since strides for length1 dimensions are meaningless and any value is valid by the numpy standard.

The bug showed up in pymc-devs/pymc#7792

This PR also introduces a benchmark test for the special logic in the cases where the gemv is doing a vector-vector dot. This was previously in the C-contiguous branch (which comes after the F-contiguous branch), which is sub-optimal because if a vector is C-contiguous it's also F-contiguous. We should apply in those cases regardless of the values the strides happen to be.

@ricardoV94 ricardoV94 added bug Something isn't working C-backend labels May 20, 2025
@ricardoV94 ricardoV94 force-pushed the fix_bug_in_gemv_c_code branch from 398ee7d to 7b750d7 Compare May 20, 2025 18:36
@ricardoV94 ricardoV94 requested a review from Copilot May 20, 2025 18:45
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug in the GEMV C code related to the handling of row/column matrices, particularly addressing the erroneous +1 addition to strides for 1×1 matrices. It also introduces additional test coverage, including a benchmark for vector–vector dot product performance.

  • Updated stride calculations in the GEMV implementation for row/column matrices
  • Added test cases and a performance benchmark for vector dot products
  • Updated error messaging and cache versioning in the GEMV code

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
tests/tensor/test_blas_c.py Added benchmark test for GEMV vector–vector dot product performance
tests/tensor/test_blas.py Included additional edge case tests (1×1 matrices)
pytensor/tensor/blas_c.py Adjusted stride calculation, branch conditions, and cache version

@ricardoV94 ricardoV94 requested review from jessegrabowski and lucianopaz and removed request for lucianopaz and jessegrabowski May 20, 2025 18:48
@ricardoV94 ricardoV94 force-pushed the fix_bug_in_gemv_c_code branch from 7b750d7 to fb59f91 Compare May 21, 2025 08:57
Bug was caused by reusing the adjusted strides in the logic to decide whether the call to GEMV should be transposed or not.

Particularly the +1 in the strides variable was causing the error branch (no double-strides) to be reached wrongly. The +1 was supposedly there for the case of matrix with length 0, but that triggers a branch where the adjusted strides are never used.

This bug was introduced in afe934b
@ricardoV94 ricardoV94 force-pushed the fix_bug_in_gemv_c_code branch from fb59f91 to 3ff344b Compare May 21, 2025 09:08
Copy link

codecov bot commented May 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.10%. Comparing base (5ffe17a) to head (3ff344b).
Report is 6 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1408   +/-   ##
=======================================
  Coverage   82.10%   82.10%           
=======================================
  Files         208      208           
  Lines       49576    49576           
  Branches     8791     8791           
=======================================
  Hits        40704    40704           
  Misses       6699     6699           
  Partials     2173     2173           
Files with missing lines Coverage Δ
pytensor/tensor/blas_c.py 92.98% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jessegrabowski jessegrabowski merged commit 709f745 into pymc-devs:main May 21, 2025
73 checks passed
@ricardoV94 ricardoV94 deleted the fix_bug_in_gemv_c_code branch May 21, 2025 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working C-backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants