Return the index of the first NaN from argmax and argmin - #4292
Closed
erwinzhang7 wants to merge 1 commit into
Closed
Return the index of the first NaN from argmax and argmin#4292erwinzhang7 wants to merge 1 commit into
erwinzhang7 wants to merge 1 commit into
Conversation
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.
Member
|
Closing as a duplicate of #4291. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4274.
Every comparison against a NaN is false, so the bare
<and>in the arg reduceoperators step over NaNs entirely:
argmaxandargminreturn the index of thelargest or smallest real value while
maxandminreturn NaN for the same input,so
a[mx.argmax(a)]andmx.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,cummaxandcummin, the last two since #4044, soargmaxandargminwere 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_manyneeded the same change asreduce. Without it the vectorised pathstill 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_vfromhalf_types.h, which covers float16 andbfloat16 where
std::is_floating_pointdoes not; Metal and CUDA use a smallarg_is_nanhelper 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_opsandtest_autogradpass, andtest_reducegains 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.