Skip to content

Return the index of the first NaN from argmax and argmin - #4292

Closed
erwinzhang7 wants to merge 1 commit into
ml-explore:mainfrom
erwinzhang7:argreduce-nan
Closed

Return the index of the first NaN from argmax and argmin#4292
erwinzhang7 wants to merge 1 commit into
ml-explore:mainfrom
erwinzhang7:argreduce-nan

Conversation

@erwinzhang7

Copy link
Copy Markdown
Contributor

Fixes #4274.

Every comparison against a NaN is false, so the bare < and > in the arg reduce
operators step over NaNs entirely: argmax and argmin return the index of the
largest or smallest real value while max and min return NaN for the same input,
so a[mx.argmax(a)] and mx.max(a) disagree on any array containing one.

numpy and torch both return the index of the first NaN, and MLX already propagates
in max, min, cummax and cummin, the last two since #4044, so argmax and
argmin were the remaining pair that did not agree with the rest.

A NaN now wins the comparison, and once one is held it is not replaced, so the result
is the first NaN rather than whichever the reduction happened to associate last.

reduce_many needed the same change as reduce. Without it the vectorised path
still skips NaNs while the scalar path does not, so the answer would depend on the
length of the array.

The float check is a compile time guard so the integer instantiations are unchanged.
CPU uses is_floating_point_v from half_types.h, which covers float16 and
bfloat16 where std::is_floating_point does not; Metal and CUDA use a small
arg_is_nan helper around the same idea.

Verified

M5 Max, cpu and metal streams: the five cases in the issue, float32, float16 and
bfloat16, lengths 8, 33, 1000 and 5000 to cover the vectorised path, an array with
two NaNs to confirm the first one wins, and integer input. Without a NaN nothing
changes, including the lowest index tie break. test_reduce, test_ops and
test_autograd pass, and test_reduce gains a case for this.

The CUDA change is the same shape but I have no CUDA machine, so it is compiled by
CI rather than run.

Every comparison against a NaN is false, so the bare < and > in the arg reduce
operators step over NaNs entirely. argmax and argmin return the index of the
largest or smallest real value while max and min return NaN for the same input,
so a[mx.argmax(a)] and mx.max(a) disagree on any array containing one.

numpy and torch both return the index of the first NaN. MLX already propagates
NaN in max, min, cummax and cummin, the last two since ml-explore#4044, so argmax and
argmin were the remaining pair that did not agree with the rest.

A NaN now wins the comparison, and once one is held it is not replaced, so the
index is the first NaN rather than whichever the reduction happened to
associate last. The float check is a compile time guard, so the integer
instantiations are unchanged and still compile: cpu uses is_floating_point_v
from half_types.h, which covers float16 and bfloat16, metal and cuda use a
small arg_is_nan helper around the same idea.

reduce_many needed the same treatment as reduce, or the vectorised path would
skip NaNs for longer arrays while the scalar path did not.

Verified on an M5 Max, cpu and metal, against the five cases in the report plus
float16 and bfloat16, lengths 8, 33, 1000 and 5000 to cover the vector path, an
array with two NaNs to check the first one wins, and integer input. Without a
NaN nothing changes, including the lowest index tie break. test_reduce,
test_ops and test_autograd pass. The cuda path is the same change but is not
built here.
@zcbenz

zcbenz commented Aug 16, 2026

Copy link
Copy Markdown
Member

Closing as a duplicate of #4291.

@zcbenz zcbenz closed this Aug 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] mx.argmax / mx.argmin skip NaN while mx.max / mx.min propagate it

2 participants