-
Notifications
You must be signed in to change notification settings - Fork 145
Handle axis=None
symbolically instead of within CumOp
#1574
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
base: main
Are you sure you want to change the base?
Conversation
@copilot address review comments |
axis=None
symbolically instead of within CumOp
@copilot some tests Other tests may be failing, check the logs to be sure it's not only this one |
Fixed the failing tests by removing negative axis cases from CumOp tests. Updated both |
aad9f7d
to
1cf93c3
Compare
1cf93c3
to
a09b463
Compare
@jessegrabowski requested your review because copilot PRs are not really someone else's? |
axis=None
symbolically instead of within CumOpaxis=None
symbolically instead of within CumOp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nitpicks
if not isinstance(axis, int): | ||
raise TypeError("axis must be an integer.") | ||
if axis < 0: | ||
raise ValueError("axis must be non-negative.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The similar change for repeat
told the user to go use the pt.repeat
helper in your axis is negative or None, that would be nice here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cumop is more annoying because user could be using cumsum or cumprod. Anyway that was a bit overkill in that PR, nobody is instantiating Ops for themselves when using PyTensor
out_type = vector(dtype=x.dtype) # Flatten | ||
elif self.axis >= x.ndim or self.axis < -x.ndim: | ||
if self.axis >= x.ndim: | ||
raise ValueError(f"axis(={self.axis}) out of bounds") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raise ValueError(f"axis(={self.axis}) out of bounds") | |
raise ValueError(f"axis(={self.axis}) out of bounds for tensor with {x.ndim} dimensions") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You didn't change this but I saw it, and the robot is doing the work anyway.
def cumsum(x, axis=None): | ||
"""Return the cumulative sum of the elements along a given `axis`. | ||
This wraps ``numpy.cumsum``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What not really :)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't wrap numpy.cumsum, it implements similar functionality though.
Anything blocking this one? |
Fixes #1549
Summary
This PR refactors the cumulative sum/product operations to handle the special ravelling behavior of
axis=None
symbolically, making the code cleaner and more maintainable.Problem
Previously, when
cumsum(x, axis=None)
orcumprod(x, axis=None)
was called on a matrix, theCumOp
class internally handled the flattening with special logic scattered throughout:make_node()
,grad()
,infer_shape()
, andc_code()
Solution
The refactoring separates concerns by handling the ravelling symbolically:
Before:
After:
Key Changes
Modified
cumsum
/cumprod
functions: Whenaxis=None
, explicitly flatten the input first, then apply cumsum/cumprod withaxis=0
Simplified
CumOp
class:axis=None
)make_node
,grad
,infer_shape
,c_code
Updated backend dispatchers: Removed axis=None handling from PyTorch, JAX, and Numba backends since
CumOp
now always receives integer axesUpdated tests: Modified test cases to use the new simplified API while maintaining coverage of axis=None behavior through the public functions
Benefits
CumOp
only handles specific integer axesflatten → cumsum
chainBackward Compatibility
✅ All existing user code continues to work unchanged
✅ Same computational results for all operations
✅ Same gradient behavior
✅ Same output shapes and types
The refactoring only changes the internal implementation - the public API remains identical.
Example
This makes the code more maintainable while preserving all existing functionality.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.