Skip to content

Conversation

jessegrabowski
Copy link
Member

@jessegrabowski jessegrabowski commented Oct 14, 2025

Description

Rename tensor.sparse.mul -> tensor.sparse.multiply and tensor.sparse.sub -> tensor.sparse.subtract

Related Issue

Checklist

Type of change

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

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

ricardoV94
ricardoV94 previously approved these changes Oct 14, 2025
@ricardoV94
Copy link
Member

hmm I'm sure that issue was meant for tensor operations not sparse. Those don't exist on numpy so we can't "match their API". How does scipy call them?

@ricardoV94 ricardoV94 self-requested a review October 14, 2025 07:25
@Armavica
Copy link
Member

Armavica commented Oct 14, 2025

I would vote for yes, scipy also calls them multiply. And as far as I understand it's not numpy's API that we want to match but the Array API which is supposed to apply to whatever behaves like an array?

But why not also change the tensor operations in tensor/math.py while we are at it?

Copy link

codecov bot commented Oct 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.57%. Comparing base (95acdb3) to head (b331f9a).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1663   +/-   ##
=======================================
  Coverage   81.56%   81.57%           
=======================================
  Files         242      242           
  Lines       53819    53827    +8     
  Branches     9485     9485           
=======================================
+ Hits        43899    43907    +8     
  Misses       7431     7431           
  Partials     2489     2489           
Files with missing lines Coverage Δ
pytensor/sparse/basic.py 82.71% <100.00%> (+0.07%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ricardoV94
Copy link
Member

We want to match numpy/scipy API, in case it doesn't have an equivalent (like subtract it seems), we can use the array API as the next best thing. That's where numpy/scipy want to converge anyway

@ricardoV94
Copy link
Member

So both changes are fine

@jessegrabowski
Copy link
Member Author

Do you want me to do the pytensor.tensor stuff in this PR too? Happy to. I did sparse in this PR because I searched for open issues related to sparse and this came up.

@ricardoV94
Copy link
Member

As you prefer, it's your work

@jessegrabowski
Copy link
Member Author

But why not also change the tensor operations in tensor/math.py while we are at it?

I started looking into this, but it seems like a pretty big job. I made the trivial replacements, then \started hitting all kinds of random bugs with rewrites and prettyprint and was quickly demotivated.

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.

Rename to multiply to match numpy

3 participants