Skip to content

Conversation

@Ray0907
Copy link

@Ray0907 Ray0907 commented Nov 2, 2025

RS_quantiles with nth_element

Replace O(n log n) std::sort with O(n) std::nth_element for quantile
computation. This addresses the TODO comment requesting quickselect.

Changes:

  • Use std::nth_element to find lower and upper quantile positions
  • Eliminates unnecessary full sorting of x_copy vector
  • Maintains identical behavior for quantile calculation
  • Theoretical performance O(n log n) → O(n)

   RS_quantiles with nth_element

   Replace O(n log n) std::sort with O(n) std::nth_element for quantile
   computation. This addresses the TODO comment requesting quickselect.

   Changes:
   - Use std::nth_element to find lower and upper quantile positions
   - Eliminates unnecessary full sorting of x_copy vector
   - Maintains identical behavior for quantile calculation
   - Theoretical performance O(n log n) → O(n)
@meta-cla meta-cla bot added the CLA Signed label Nov 2, 2025
@meta-codesync
Copy link
Contributor

meta-codesync bot commented Nov 4, 2025

@limqiying has imported this pull request. If you are a Meta employee, you can view this in D86234649.

@subhadeepkaran
Copy link

subhadeepkaran commented Nov 7, 2025

feels like a corner case that needs to be handled here when o > n/2
then it opens the scenario for being vmin > vmax

i think we should ensure: o = std::clamp(int(rs_arg * n), 0, n/2);

i can put out a follow-up diff on this:

@Ray0907
Copy link
Author

Ray0907 commented Nov 8, 2025

You're right there's an issue. However, the correct bound should be (n-1)/2 instead of n/2 to ensure vmin_index <= vmax_index (i.e.,
o <= n-1-o). I've updated the fix accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants