Skip to content
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

implement Statistics.median #973

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

stev47
Copy link
Contributor

@stev47 stev47 commented Nov 29, 2021

This implements Statistics.median based on the existing bitonic
sorting, avoiding unnecessary allocation.
While it is generally suboptimal to sort the whole array, the compiler
manages to skip some branches since only the middle element(s) are used.
Thus median is generally faster than sort.

Using a dedicated median selection network could yield better
performance and might be considered for future improvement.

@RoyiAvital
Copy link

Any chance to push this forward?

This implements `Statistics.median` based on the existing bitonic
sorting, avoiding unnecessary allocation.
While it is generally suboptimal to sort the whole array, the compiler
manages to skip some branches since only the middle element(s) are used.
Thus `median` is generally faster than `sort`.

Using a dedicated median selection network could yield better
performance and might be considered for future improvement.
@stev47
Copy link
Contributor Author

stev47 commented Oct 29, 2024

Any chance to push this forward?

updated the MR, waiting for review

@mkitti
Copy link

mkitti commented Nov 12, 2024

This pull request implements median for StaticVector. It supports the empty array, missing, and NaN edge cases. For regular functionality, the tests includes a comparison to the Vector equivalent of a random StaticVector

While testing random static vectors is useful, defined tests would be welcome. In particular, one could replicate the the tests in Statistics.jl for median:
https://github.com/JuliaStats/Statistics.jl/blob/d49c2bf4f81e1efb4980a35fe39c815ef8396297/test/runtests.jl#L31-L92

@stev47
Copy link
Contributor Author

stev47 commented Nov 23, 2024

[...] defined tests would be welcome. In particular, one could replicate the the tests in Statistics.jl for median:

ok, I've added the tests from Statistics.jl

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.

3 participants