-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
fix: Check shape for *_horizontal
functions
#20130
fix: Check shape for *_horizontal
functions
#20130
Conversation
Fixes pola-rs#19336. There was some unsoundness before when creating the frame that has now be resolved. This also moves pl.DataFrame.*_horizontal functions away from their own custom implementation and towards using `pl.select(_ = pl.*_horizontal)`.
*_horizontal
functions
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20130 +/- ##
=======================================
Coverage 79.52% 79.52%
=======================================
Files 1563 1564 +1
Lines 217184 217194 +10
Branches 2465 2465
=======================================
+ Hits 172706 172728 +22
+ Misses 43917 43905 -12
Partials 561 561 ☔ View full report in Codecov by Sentry. |
let sum = || sum_horizontal(&columns, null_strategy); | ||
let null_count = || { | ||
columns | ||
.par_iter() |
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.
Do we need this in core, if we could store it in polars-ops
that wuold be preferred.
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.
It would remove the sum_horizontal
, mean_horizontal
, ... methods from the DataFrame methods. If you are okay with that, I can do it.
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.
We could TraitExt
them in ops?
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.
We could decide to do that in a future PR. I think it's worth it.
Fixes #19336.
There was some unsoundness before when creating the frame that has now be resolved.
This also moves pl.DataFrame.*_horizontal functions away from their own custom implementation and towards using
pl.select(_ = pl.*_horizontal)
.