Repository navigation
[DF] Add median support for RDataFrame - #23079
Conversation
|
Hi @ArjunXvarma , Thanks for your contribution! I will go ahead and approve the CI run, meanwhile I'll take a look at the changes and comment accordingly. |
Test Results 23 files 23 suites 3d 13h 48m 28s ⏱️ Results for commit 97cca4c. ♻️ This comment has been updated with latest results. |
|
Hi @vepadulano, I have rebased onto current master to pick up the recent CI changes. Could you please approve the workflow run? |
vepadulano
left a comment
There was a problem hiding this comment.
Thank you for this effort! Here are some comments from my side
|
Thanks for the update! The CI seems to be green. Before proceeding with this PR, can you still consider the addition of the two tests I mentioned in my above comment? |
|
Hi @vepadulano, I'm currently working on the tests, thought I'd make separate commits for the class changes and test additions for the sake of readability. |
|
@vepadulano, I've added the tests you suggested.
I compared this against Mean on the same column: both are rejected at the |
This commit introduces the Median lazy action, which computes the median of the values in the input column. Median always returns an RResultPtr<double> irrespective of the input column type. If the input column is of a type which does not make sense to compute the median on, the action will throw a std::runtime_error during execution when the input value is tried to be coerced into a double. The current implementation works for local RDataFrame in single- and multi-threaded mode. It stores all column values in a buffer and computes the exact median at the end of processing.
vepadulano
left a comment
There was a problem hiding this comment.
Thank you for the nice work @ArjunXvarma !
I have rebased your branch squashing all commit history and adding a commit description of the feature introduced.
No further action is required on your side, I will merge this PR once the tests pass.
This Pull request:
Changes or fixes:
Adds a
Medianaction to RDataFrame, computing the exact median of a column.Draft - implementation is working; Google Test coverage and doxygen documentation still to come.Implemented as a
MedianHelperinActionHelpers.hxxfollowing the structure ofMeanHelper:values are buffered per slot in
Exec(with anIsDataContaineroverload for collection columns),and
Finalizemerges the per-slot buffers and usesstd::nth_elementto select the middleelement. Even entry counts return the average of the two middle values.
Scope, per the discussion in the issue: exact mode only.
GetMergeableValueis not implemented,so distributed execution is not supported - a median cannot be merged from a summary the way
RMergeableMeanmerges(mean, count), so that deserves separate discussion. Memory cost isO(N) doubles, which is inherent to an exact result.
Manually verified so far via the templated and jitted C++ paths, from Python, on
RVeccolumns,and with implicit multithreading enabled.
Checklist:
This PR is part of #22794