fix: int64 precision loss in sort comparators and per-comparison branching#4090
Open
henryiii wants to merge 4 commits into
Open
fix: int64 precision loss in sort comparators and per-comparison branching#4090henryiii wants to merge 4 commits into
henryiii wants to merge 4 commits into
Conversation
henryiii
added a commit
that referenced
this pull request
Jun 10, 2026
Tests that ak.sort and ak.argsort on int64/uint64 values > 2^53 match numpy exactly, verifying the double-cast precision bug is fixed. Also covers NaN float handling (unchanged), bool sort, descending order, stable sort, and multiple sublists. Assisted-by: ClaudeCode:claude-sonnet-4-6
Member
|
@henryiii — let’s discuss the merge order for kernels at the meeting tomorrow |
…ching - Use `if constexpr (std::is_integral_v<T>)` in sort_order_ascending/ descending and argsort_order_ascending/descending so integer types compare directly without double-casting (fixes precision loss for int64/uint64 values > 2^53, and eliminates two casts + isnan per comparison for integral types). - Move explicit `template<>` bool specializations above the implicit instantiations that use them, fixing ill-formed translation units per [temp.expl.spec]. - Hoist ascending/stable dispatch outside the comparator lambda in awkward_argsort, matching the structure of awkward_sort so the branch is evaluated once per segment rather than once per comparison. Assisted-by: ClaudeCode:claude-sonnet-4-6
Tests that ak.sort and ak.argsort on int64/uint64 values > 2^53 match numpy exactly, verifying the double-cast precision bug is fixed. Also covers NaN float handling (unchanged), bool sort, descending order, stable sort, and multiple sublists. Assisted-by: ClaudeCode:claude-sonnet-4-6
- Remove descriptive comments in C++ that restate what code does; keep the non-obvious constraint comment about specialization ordering. - Trim 13 regression tests to 5: one per distinct concern (int64 sort, int64 argsort, uint64 sort, float NaN unchanged, bool specialization). - Rename test file to use underscores to pass validate-test-names hook. Assisted-by: ClaudeCode:claude-sonnet-4-6
henryiii
force-pushed
the
henryiii/fix-sort-comparators
branch
from
9e85a02 to
60c6f74
Compare
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
2 participants
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.
🤖 AI text below 🤖
Summary
Three correctness and performance fixes in `awkward-cpp/src/cpu-kernels/`:
`awkward_sort.cpp` and `awkward_argsort.cpp` (lines 12-22): The `sort_order_ascending`/`sort_order_descending` and `argsort_order_ascending`/`argsort_order_descending` templates cast both operands to `double` and call `std::isnan` for every comparison, even for integer types. This is (a) a correctness bug for `int64`/`uint64` values beyond 2^53 — double rounding can compare distinct integers as equal, breaking strict-weak-ordering — and (b) two int→double casts plus `isnan` overhead per comparison. Fixed by adding `if constexpr (std::is_integral_v) { return l < r; }` (resp. `>`), preserving the NaN-aware path only for floating-point types.
Explicit `template<>` bool specializations (end of each file): Were declared after the implicit instantiations that use them, which is ill-formed per [temp.expl.spec] (no diagnostic required). Moved above the first use, with forward declarations for the primary templates.
`awkward_argsort.cpp` comparator lambda (lines 42-51): Branched on `ascending` inside every comparator call — O(n log n) evaluations. Restructured to hoist the `ascending`/`stable` dispatch outside the sort call, matching the existing structure of `awkward_sort.cpp`.
Note: `awkward-cpp` version bump will happen at release time per project convention.
Changes
Test plan
AI assistance
This PR was generated with Claude Code (automated multi-agent), claude-sonnet-4-6. Per CONTRIBUTING.md, AI assistance is disclosed here.
🤖 Generated with Claude Code