fix: replace hand-rolled quickselect with select_nth_unstable_by#407
Open
agene0001 wants to merge 1 commit into
Open
fix: replace hand-rolled quickselect with select_nth_unstable_by#407agene0001 wants to merge 1 commit into
agene0001 wants to merge 1 commit into
Conversation
`select_inplace` implemented the Numerical Recipes quickselect by hand. With NaN in the slice its partition loop walks past the end of the array, which is the crash reported in statrs-dev#163 (a segfault under the older unsafe version, an index-out-of-bounds panic today): let mut v: Vec<f64> = (0..1000).map(|i| (i as f64 * 0.7).sin()).collect(); for i in (0..1000).step_by(7) { v[i] = f64::NAN; } Data::new(v).quantile(0.5); // panics on main `<[T]>::select_nth_unstable_by` with `total_cmp` fixes this three ways: NaN gets a defined position in a total order, the worst case is O(n) rather than quadratic, and it is ~4.4x faster at 1e6 elements. Closes statrs-dev#163
agene0001
added a commit
to agene0001/statrs
that referenced
this pull request
Jul 26, 2026
Brings the integration branch to the exact union of PRs statrs-dev#407-statrs-dev#420: adds the NaN regression test written for statrs-dev#407 (statrs-dev#163), relocates the Geometric near-1 test to match the prec PR, and aligns two comment/tolerance details.
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.
select_inplaceimplements the Numerical Recipes quickselect by hand. With NaN in the slice, its partition loop walks past the end of the array — the crash reported in #163 (a segfault under the pre-0.13unsafeversion, an index-out-of-bounds panic today). Reproducer that panics onmain:Replacing it with
<[T]>::select_nth_unstable_by(rank, f64::total_cmp)fixes it three ways:The regression test covers NaN scattered through a large slice, all-NaN input, and NaN at the exact positions the old partition tripped on. Existing order-statistic/median/quantile tests (including
test_median_robust_on_infinities) pass unchanged.Closes #163
🤖 Generated with Claude Code