Start complex min and max scans from an infinite identity - #4272
Open
ayaangazali wants to merge 2 commits into
Open
Start complex min and max scans from an infinite identity#4272ayaangazali wants to merge 2 commits into
ayaangazali wants to merge 2 commits into
Conversation
An exclusive scan writes the identity of its operation into the first position. That identity came from std::numeric_limits, which has no specialization for complex64_t and so handed back zero. Zero then won every comparison against a negative real part, and cummax and cummin returned all zeros. The CUDA backend already uses an infinite pair.
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.
An exclusive scan writes the identity of its operation into the first position. On the CPU that identity is picked like this:
complex64isinexactbut notfloating, so it takes the second branch.std::numeric_limitshas no specialization forcomplex64_t, somax()returns a value initializedcomplex64_t, which is0+0j. Complex comparison is lexicographic on the real part, so that zero beats every negative real part and the scan never recovers:cummingoes the same way whenever the data is non negative, andlogcumsumexpstarts from0+0jinstead of-inf:The inclusive results are correct throughout, so the two disagree: an exclusive scan should equal the inclusive one shifted by a position, and for complex it does not.
Neither GPU backend has this problem: both Metal (
mlx/backend/metal/kernels/utils.h) and CUDA (mlx/backend/cuda/device/utils.cuh) specializeLimits<complex_t<T>>to an infinite pair, soCumMaxthere already starts from{-inf, -inf}andCumMinfrom{inf, inf}. The CPU was the only backend without it. This gives the CPU the same identity.scan_extremereplaces the three copies of the ternary, and picks the pair{inf, inf}for complex so the identity also wins against an input whose real part is itself infinite, which a bareinf+0jwould lose to on the imaginary tiebreak.logcumsumexpkeeps its own ternary, widened toinexactso complex reaches-inf. It deliberately does not usescan_extreme: its identity has to compare equal to theminval == -inftest insideLogAddExp, and an infinite imaginary part would fail that and produce NaN.Verified that exclusive equals inclusive shifted by one across
cumsum,cumprod,cummax,cumminandlogcumsumexp, over 8 dtypes, shapes(7,),(3,5),(2,3,4),(1,)and(9,2), every axis, and both scan directions. Complex goes from broken to matching; the real dtypes are byte identical to before and still match numpy.Out of scope and left alone:
logcumsumexpon an unsigned integer input starts from 0 because there is no representable-inf, which is a different problem and predates this.CPU only.
test_ops.py,test_reduce.py,test_array.py,test_compile.py,test_vmap.py,test_autograd.py,test_nn.pyand the C++ suite (249 cases, 3350 assertions) pass. The added test fails on main withcummax reverse=False.I am a freshman working through this codebase and I used Claude Code alongside it. Every result above came from a run here.