Honour keepdims when axis is None in the torch backend reductions - #23535
Honour keepdims when axis is None in the torch backend reductions#23535Sid-5137 wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request simplifies several reduction operations (max, min, amax, amin, sum) in the PyTorch backend by removing redundant axis checks, and fixes prod to correctly handle keepdims=True when axis=None. A new test suite is also added to verify rank preservation for these reductions. The feedback suggests converting axis=None to a tuple of all dimensions in amax and amin to prevent potential PyTorch overload resolution issues when dim=None is passed.
| x = convert_to_tensor(x) | ||
| if axis is None: | ||
| return torch.amax(x) | ||
| if axis == () or axis == []: |
There was a problem hiding this comment.
In PyTorch, passing dim=None to torch.amax can be problematic because of PyTorch's overload resolution rules (where dim=None may not be accepted or may cause keepdim to be ignored in some PyTorch versions). Converting axis=None to a tuple of all dimensions (tuple(range(x.ndim))) is a more robust approach that is universally supported across all PyTorch versions and guarantees that the correct multi-dimensional reduction overload is matched.
| x = convert_to_tensor(x) | |
| if axis is None: | |
| return torch.amax(x) | |
| if axis == () or axis == []: | |
| x = convert_to_tensor(x) | |
| if axis is None: | |
| axis = tuple(range(x.ndim)) | |
| if axis == () or axis == []: |
| x = convert_to_tensor(x) | ||
| if axis is None: | ||
| return torch.amin(x) | ||
| if axis == () or axis == []: |
There was a problem hiding this comment.
In PyTorch, passing dim=None to torch.amin can be problematic because of PyTorch's overload resolution rules (where dim=None may not be accepted or may cause keepdim to be ignored in some PyTorch versions). Converting axis=None to a tuple of all dimensions (tuple(range(x.ndim))) is a more robust approach that is universally supported across all PyTorch versions and guarantees that the correct multi-dimensional reduction overload is matched.
| x = convert_to_tensor(x) | |
| if axis is None: | |
| return torch.amin(x) | |
| if axis == () or axis == []: | |
| x = convert_to_tensor(x) | |
| if axis is None: | |
| axis = tuple(range(x.ndim)) | |
| if axis == () or axis == []: |
On the torch backend, `keepdims=True` was silently ignored when
`axis=None` for sum, prod, max, min, amax and amin. A (2, 3) input
returned a scalar instead of shape (1, 1), so downstream code that
relies on the rank being preserved got the wrong shape with no error.
The numpy and jax backends already behave correctly, and so do
mean, std, var, any and all on the torch backend itself, which is
what made this inconsistent rather than merely undocumented.
torch, axis=None, keepdims=True, input (2, 3):
sum, prod, max, min, amax, amin -> () before
mean, std, var, any, all -> (1, 1)
Each function took a separate `axis is None` branch that dropped
keepdim. torch.sum, torch.amax and torch.amin all accept dim=None and
handle keepdim correctly, so those branches are simply removed.
torch.prod does not accept dim=None, so it reduces everything and then
restores the rank. max and min had their own bypass calling torch.max /
torch.min directly rather than going through amax / amin, so they now
route through those.
Adds a regression test covering all six ops. It fails before this
change with `Tuples differ: () != (1, 1)`.
ab8d055 to
e6fc36b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #23535 +/- ##
==========================================
- Coverage 85.02% 84.29% -0.74%
==========================================
Files 468 468
Lines 71087 71141 +54
Branches 11788 11788
==========================================
- Hits 60442 59966 -476
- Misses 7632 8174 +542
+ Partials 3013 3001 -12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Addresses review feedback: older torch versions do not accept dim=None for torch.amax / torch.amin, so reduce over an explicit tuple of all dimensions instead. Behaviour on current torch is unchanged, verified against numpy for 0-d, 1-d and 2-d inputs.
The openvino backend has its own axis=None + keepdims=True defects (sum, prod, amax, amin, std, var, any and all return shape (1,) instead of preserving the rank, and mean raises AttributeError), so the new test cannot pass there yet. Excluded via the existing openvino skip list; the backend gap is reported separately.
hertschuh
left a comment
There was a problem hiding this comment.
Thanks for catching this!
| if isinstance(getattr(result, "values", None), torch.Tensor): | ||
| result = result.values |
There was a problem hiding this comment.
This is no longer needed, it was an artifact of using torch.max I believe.
| if isinstance(getattr(result, "values", None), torch.Tensor): | ||
| result = result.values |
| def test_reductions_keepdims_with_axis_none(self): | ||
| # `keepdims=True` must preserve the rank even when `axis=None`, | ||
| # matching numpy. The tests above only cover an explicit axis. | ||
| x = np.array([[1, 2, 3], [3, 2, 1]]) | ||
| for op_name in ("sum", "prod", "max", "min", "amax", "amin"): | ||
| knp_op = getattr(knp, op_name) | ||
| np_op = getattr(np, op_name) | ||
| expected = np_op(x, axis=None, keepdims=True) | ||
| result = knp_op(x, axis=None, keepdims=True) | ||
| self.assertEqual( | ||
| tuple(result.shape), | ||
| expected.shape, | ||
| msg=f"{op_name}(axis=None, keepdims=True) shape mismatch", | ||
| ) | ||
| self.assertAllClose(result, expected) |
There was a problem hiding this comment.
Instead of this, please add one test case in each one of the existing test functions for each op. Don't add a new test function just add a line in the existing one.
The reason is that we're going to split tests by op and tests like this one won't play nice with this refactor.
Thanks!
…functions - max and min: the .values unwrapping was only ever needed for torch.max(x, dim=...), which returns a namedtuple. amax and amin always return a plain tensor, so the branch was unreachable. Removed, and both now route straight through amax / amin. - The combined test_reductions_keepdims_with_axis_none is gone. Each of the existing per-op tests (sum, prod, max, min, amax, amin) now covers axis=None with keepdims=True inline. assertAllClose broadcasts a scalar against (1, 1) and would pass on the broken code, so each op asserts the output shape explicitly first. Verified the tests fail without the backend fix and pass with it. - The openvino skip moved from excluded_concrete_tests.txt to inline backend guards on the four ops openvino gets wrong, with a TODO pointing at keras-team#23536. max and min pass on openvino and are unguarded. excluded_concrete_tests.txt is back to untouched.
|
Thanks for the review! All three changes are done.
|
Description
On the torch backend,
keepdims=Trueis silently ignored whenaxis=Noneforsum,prod,max,min,amaxandamin. A(2, 3)input returns a scalar instead of shape(1, 1), so code that relies on the rank being preserved gets the wrong shape with no error.numpy and jax are already correct, and so are
mean,std,var,anyandallon the torch backend itself, which is what makes this an inconsistency rather than an undocumented limitation.Cause
Each of the six took a separate
axis is Nonebranch that never forwardedkeepdim:torch.sum,torch.amaxandtorch.aminall acceptdim=Noneand honourkeepdimcorrectly, so those branches are simply removed.torch.prodis the exception, itsdimmust be an int and it rejectsdim=None, so it reduces everything and then restores the rank whenkeepdims=True.maxandminhad their own bypass callingtorch.max/torch.mindirectly instead of going throughamax/amin, so fixingamax/aminalone did not fix them. They now route through those.Tests
Adds
NumpyOneInputOpsCorrectnessTest::test_reductions_keepdims_with_axis_none, covering all six ops against numpy. The existing tests only ever exercise an explicit axis, which is why this was not caught.Verified the test actually catches the bug: reverting just the backend change and keeping the test gives
On the openvino backend the four affected assertions are guarded inline with a
TODO(#23536): that backend has its own, separateaxis=None+keepdimsdefects and cannot pass them yet. This PR only touches the torch backend, so fixing those is out of scope here, and the guards can be dropped once #23536 is resolved.Verification
Full
keras/src/opssuite:ruff checkandruff format --checkare clean.AI assistance disclosure
Per the AI-Assisted Contribution Policy: I used Claude Code on this change. It found the inconsistency by comparing reduction behaviour across backends, traced each of the separate
axis is Nonebranches, drafted the fix and the regression test, and ran the suites locally. I reviewed the change, I understand whytorch.prodneeds different handling from the others and whymax/minneeded fixing separately, and I will be answering review comments myself.Contributor Agreement