fix(ops): propagate NaN through median - #4146
Open
devteamaegis wants to merge 1 commit into
Open
Conversation
median sorts and takes the midpoint. Sorting moves NaN to the end of the axis, so the midpoint slice never selects it and the NaN is silently dropped. Mask the result on any(isnan(...)) over the reduced axes for inexact dtypes, which matches max, min, mean, cummax and cummin, as well as NumPy and PyTorch.
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.
Proposed changes
What's broken.
mx.mediansilently drops NaN — it returns a real number for input that contains NaN.Reproduced on CPU (
mlx 0.32.1.dev20260810+e78d894, source build,-DMLX_BUILD_METAL=OFF). Both NumPy and PyTorch returnnanhere:mx.mediantorch.mediannp.median[1.0, nan, 0.0][nan, 1.0, 0.0][-5.0, nan, 3.0][1.0, 0.0, nan]It is also inconsistent inside MLX:
max,min,mean,cummaxandcumminall propagate NaN, somedianis the odd one out among the reductions.The behaviour is shape-dependent, which makes it easy to miss.
medianover an axis of even length can average the NaN in by accident, so the same array gives a NaN along one axis and a plausible-looking number along another:Why.
mediansorts the reduced axes and slices the midpoint (mlx/ops.cpp).sortmoves NaN to the end of the axis, so for an odd-length axis the midpoint is always a non-NaN element and the NaN is never observed.The fix. After taking the midpoint, mask the result where the reduced axes contain a NaN. Guarded on
issubdtype(a.dtype(), inexact), so integer input (which is promoted to float but can never be NaN) keeps the original code path. Complex is covered too, matching NumPy's(nan+0j).The test.
test_median_naninpython/tests/test_ops.py, covering odd/even axis lengths, NaN in leading/middle/trailing position,float16/bfloat16/float32, per-axis and all-axes reductions,keepdims, complex, and negative controls (NaN-free float input and integer input are unchanged).Fails before, passes after:
python/tests/test_ops.py,test_autograd.pyandtest_reduce.pyare green (215 passed, 5422 subtests). The rest of the suite is green apart from 18 pre-existingMetal DLPack import is not availablefailures from my CPU-only build, which this change does not touch.Benchmark.
medianalready does an O(n log n) sort, so the added O(n)isnan+anypass is small. CPU, M4, best of 3 runs, usingbenchmarks/python/time_utils.py:median((1_000_000,))all axesmedian((1024,1024))all axesmedian((1024,1024), axis=1)median((256,256,64), axis=(0,2))median((64,64,64,64), axis=(1,3))median((1024,1024))int32, all axesThe int32 row takes the guarded path, so it runs byte-identical code in both builds; its +2.8% is the run-to-run noise floor on this machine. Everything except the largest 4-D multi-axis reduce sits inside that noise, and that case is +6.2%.
Checklist
pre-commit run --all-filesto format my code / installed pre-commit prior to committing changes