Raise IndexError for out of bounds axes - #4484
Conversation
An out of bounds axis is an indexing error, but most axis checks threw std::invalid_argument, which nanobind surfaces as ValueError. The reduction path already threw std::out_of_range (IndexError), so mx.sum(x, axis=5) and mx.expand_dims(x, 5) disagreed on the same mistake. numpy raises AxisError for both, which subclasses ValueError and IndexError. Switch the bounds checks to std::out_of_range. Most of them go through normalize_axis_index, the rest are ad hoc checks in ops, fft, random and the vmap bindings. Errors that are not about bounds, duplicate axes, axis count mismatches and shape mismatches, stay ValueError. Closes ml-explore#4428
|
PR goes against the contributing guidelines related to AI usage policy. (Description is AI generated). This PRs bring up some breaking changes. Ideally I'd prefer waiting for discussion or solution review in original thread before going for changes directly. |
|
@ev-br Can you please check if this would be expected change for Array API compatibility? |
|
The Array API spec rarely mandates a specific exception type. A typical requirement is If provided an invalid axis, the function must raise an exception. (e.g. moveaxis, concat, repeat). One relatively rare exception is expand_dims, which additionally mandates that If provided an invalid axis, an IndexError should be raised. --- note the should not must. Therefore, changing ValueErrors into IndexErrors is a step towards Array API compatibility indeed, even if not a critical one. |
the patch. The direction, the scope calls and the review responses are mine,
and I verified the result myself: full Python and C++ suites, confirmed the
new test fails on main, and checked each flipped site is a bounds error
rather than some other argument error. I am responsible for every line.
Closes #4428.
An out of bounds axis is an indexing error, but most of the axis checks throw
std::invalid_argument, which reaches Python asValueError. The reduction pathalready throws
std::out_of_range, so the same mistake gives two differentexceptions depending on which op you call.
numpy raises
AxisErrorfor both, which subclassesValueErrorandIndexError.What changed
Bounds checks now throw
std::out_of_rangeand reach Python asIndexError.Most go through
normalize_axis_index, which covers 28 call sites on its own.The rest are ad-hoc checks in
ops.cpp,fft.cpp,random.cppand the vmapbindings. Errors that are not about bounds stay
ValueError, so duplicate axes,rank mismatches and empty arrays are untouched.
Breaking change
IndexErroris not a subclass ofValueError, so code catchingValueErrorona bad axis stops catching it. I raised the options in #4428 and will match
whatever is decided there.
Testing
New test in
python/tests/test_ops.pyasserting every op agrees onIndexErrorfor an out of bounds axis. It fails on main. Full Python suite passes with 895
tests, C++ suite with 259 cases and 3530 assertions. Built with
MLX_BUILD_METAL=OFFso the Metal kernels were not compiled locally.uvx pre-commit run --allis clean.