Skip to content

fix: replace hand-rolled quickselect with select_nth_unstable_by#407

Open
agene0001 wants to merge 1 commit into
statrs-dev:mainfrom
agene0001:fix/163-quickselect-nan
Open

fix: replace hand-rolled quickselect with select_nth_unstable_by#407
agene0001 wants to merge 1 commit into
statrs-dev:mainfrom
agene0001:fix/163-quickselect-nan

Conversation

@agene0001

Copy link
Copy Markdown

select_inplace implements 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.13 unsafe version, an index-out-of-bounds panic today). Reproducer that panics on main:

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); // panicked at src/statistics/slice_statistics.rs:106:28

Replacing it with <[T]>::select_nth_unstable_by(rank, f64::total_cmp) fixes it three ways:

  • NaN gets a defined position in a total order, so the search stays in bounds (previous behaviour with NaN was arbitrary).
  • Worst case is O(n) (introselect) instead of the NR quickselect's quadratic worst case.
  • ~4.4x faster on 1e6 uniform elements (4.49 ms → 1.02 ms including the clone), ~1.7x at 1e3.

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

`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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

segfault in select_inplace from v0.12

1 participant