h2oai / datatable

A Python package for manipulating 2-dimensional tabular data structures
https://datatable.readthedocs.io
Mozilla Public License 2.0
1.82k stars 157 forks source link

Refactor `mean()` reducer to use `FExpr` #3402

Closed samukweku closed 1 year ago

samukweku commented 1 year ago

WIP for #2562

samukweku commented 1 year ago

@oleksiyskononenko I do not understand the rules for void when computing the mean. If you have some time, I'd love your feedback on it

oleksiyskononenko commented 1 year ago

@samukweku So what is your question on voids?

oleksiyskononenko commented 1 year ago

@samukweku I see your problem now. I guess our existing mean() implementation is not consistent when it comes to groupby context. For void columns

On this PR we have already fixed that inconsistency and now only the failing tests have to be fixed. Go ahead and adjust the types in the reference frames to dt.float64.

oleksiyskononenko commented 1 year ago

Is there anything else you're planning to do on this PR? Not sure why the build is failing.

samukweku commented 1 year ago

@oleksiyskononenko Just ur review and merge is what's left

oleksiyskononenko commented 1 year ago

The build is read, so we first need to make it green before merging. Can you re-trigger the build to see if it was a temp glitch on AppVeyor?

samukweku commented 1 year ago

Sure, will do

oleksiyskononenko commented 1 year ago

@samukweku once I merge https://github.com/h2oai/datatable/pull/3412, please remove your changes to test_median* tests and merge main onto your PR. test_median* tests have typos and testing mean() instead of median().

oleksiyskononenko commented 1 year ago

Nevermind, I've already made the required fixes. See if you have anything else to be done for this PR, otherwise, I guess it is all good to be merged.

samukweku commented 1 year ago

Thanks @oleksiyskononenko all good from me. Ok to merge