-
Notifications
You must be signed in to change notification settings - Fork 181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace the call to sort by select in stdlib_stats_median #584
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much!
I just realised that it will also be necessary to update the specs. At the moment the specs indicated that after a call to |
…) already contains the second smallest value we want
Indeed, I forgot it. It is now done. Note that the input array is an |
The side effect of having a modified input array is actually beneficial to the speed of KdTrees once those are implemented or any other recursive partitioning that uses the median. |
@leonfoks In this case, one could just use However that is not exactly the same as computing the median (if there are an even number of elements). Does the KdTree application need this variant of the median, or does the result of |
Good point @gareth-nx ! I forgot that median was wrapping the select methods... Ignore my last comment! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you!
Based on the performance tests reported in stdlib_selection.md, I replaced all calls to
sort
byselect
instdlib_stats_median.fypp
.