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

Deprecate batched blas stuff #1215

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Feb 17, 2025

This should be merged after #1194 to avoid conflicts.

Closes #608


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

Copy link

codecov bot commented Feb 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.00%. Comparing base (c4fb0cf) to head (7a47d53).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1215      +/-   ##
==========================================
+ Coverage   81.99%   82.00%   +0.01%     
==========================================
  Files         188      188              
  Lines       48566    48514      -52     
  Branches     8677     8657      -20     
==========================================
- Hits        39823    39786      -37     
+ Misses       6580     6573       -7     
+ Partials     2163     2155       -8     
Files with missing lines Coverage Δ
pytensor/tensor/blas.py 73.55% <100.00%> (-0.40%) ⬇️
pytensor/tensor/math.py 93.24% <100.00%> (+1.23%) ⬆️
pytensor/tensor/rewriting/blas.py 89.64% <100.00%> (ø)

@ricardoV94 ricardoV94 force-pushed the deprecate_batched_blas_stuff branch from c58f0be to 7a47d53 Compare February 20, 2025 14:28
@ricardoV94 ricardoV94 marked this pull request as ready for review February 20, 2025 14:28
Copy link
Contributor

@lucianopaz lucianopaz left a comment

Choose a reason for hiding this comment

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

Lgtm except for the test cases that you removed. Why did you have to do that? Maybe it was because there were some redundant tests. If it was because some test stopped working with the changes, then I don’t think that’s right

@ricardoV94
Copy link
Member Author

Lgtm except for the test cases that you removed. Why did you have to do that? Maybe it was because there were some redundant tests. If it was because some test stopped working with the changes, then I don’t think that’s right

No everything works, but those wouldn't be testing batched_dot (the specialized Op) that works with one single batch dimension. They would be testing blockwise matmul or sometimes batched_dot after the rewrite we have if there's no broadcasting needed.

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

Successfully merging this pull request may close these issues.

Deprecate pytensor.tensor.math._tensordot_as_dot and the methods that use it
2 participants