-
Notifications
You must be signed in to change notification settings - Fork 139
Handles 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
…using flatten Co-authored-by: ricardoV94 <28983449+ricardoV94@users.noreply.github.com>
@copilot address review comments |
…ive axis handling Co-authored-by: ricardoV94 <28983449+ricardoV94@users.noreply.github.com>
… eliminate code duplication Co-authored-by: ricardoV94 <28983449+ricardoV94@users.noreply.github.com>
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 |
- Updated TestCumOp.test_grad to only test non-negative axis values when directly instantiating CumOp - Updated Numba CumOp test to remove axis=None and axis=-1 cases that fail with new CumOp constructor - CumOp constructor now rejects negative axis values, but helper functions handle axis normalization Co-authored-by: ricardoV94 <28983449+ricardoV94@users.noreply.github.com>
Fixed the failing tests by removing negative axis cases from CumOp tests. Updated both |
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.