Fix stack overflow in quantile_mut / selection routines on large constant arrays (#86)#116
Open
teddytennant wants to merge 1 commit into
Open
Conversation
The quickselect-based selection routines in `src/sort.rs` recursed once per partition step. On pathological inputs such as a large constant (all-equal) array, Hoare's partition peels off only a single element at each step, so the recursion depth grows linearly with the array length. For large arrays this overflows the thread stack and aborts the process (e.g. taking the median of `Array1::<N64>::ones(100_000)`). Rewrite `_get_many_from_sorted_mut_unchecked` and `Sort1dExt::get_from_sorted_mut` to recurse into the sub-problem with fewer array elements and iterate on the larger one, the standard technique that bounds the recursion depth to O(log n). The computed results are unchanged; only the stack usage is bounded. Closes rust-ndarray#86
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.
What
Rewrites the quickselect-based selection routines in
src/sort.rsso that recursion depth is bounded to O(log n):_get_many_from_sorted_mut_uncheckednow recurses into the sub-problem with fewer array elements after eachpartition_mutand iterates (via aloop) on the larger one, instead of recursing into both sides.Sort1dExt::get_from_sorted_mutnow narrows its view in place inside aloop(viaslice_axis_inplace) instead of recursing.Adds a regression test in
tests/quantile.rs.Why
Both routines previously recursed once per partition step. On a large constant (all-equal) array, Hoare's
partition_mutplaces the pivot at index 0, so each step peels off exactly one element and recurses on the remainingn - 1— an O(n) recursion depth. For large inputs (e.g. the median ofArray1::<N64>::ones(100_000)) this overflows the thread stack and aborts the whole process rather than returning a result.Recursing only into the smaller partition and iterating on the larger one is the standard technique for bounding quickselect's stack depth to O(log n). The rearranged elements and returned order statistics are unchanged; only stack usage is bounded.
Fixes #86.
Testing
quantile_mut_on_large_constant_array_does_not_overflow_stack, which aborts withthread has overflowed its stackonmasterand passes with this change.cargo testpasses in full, including the existingquickcheckquantile properties (test_quantiles_mut,test_quantiles_axis_mut) that compare bulk vs. single-quantile results, and theget_many_from_sorted_mutunit tests insrc/sort.rs.cargo fmt --checkis clean.