-
Notifications
You must be signed in to change notification settings - Fork 147
feat: when-then broadcasting #2663
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
feat: when-then broadcasting #2663
Conversation
023a936
to
9607f7b
Compare
Rebased on top of #2662 do not merge until that PR lands in main. |
thanks @camriddell ! could you rebase again please? |
9607f7b
to
2f9edfc
Compare
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.
wow!!!!! well done!
narwhals/_dask/namespace.py
Outdated
|
||
if self._otherwise_value is None: | ||
return [then_series.where(condition)] | ||
otherwise_value = get_dask_expr()._expr.Where._defaults["other"] |
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.
this is the only part that jumps out to me as suspicious π€
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 @camriddell ! really solid work here! really impressed you figured out all the details, and I'm excited that we can support nw.when(nw.col('a').count()>0).then(nw.col('a').sum())
now π₯³
I just made two updates:
- not using private methods from Dask. In general, doing that kind of thing gives a very high chance of future failures, i'd advising against doing that whenever possible
- replace a couple of
if x is True
with justif x
(unless there was some reason i missed for usingif x is True
?
What type of PR is this? (check all applicable)
Related issues
Checklist
If you have comments or can explain your changes, please do so below
This adds broadcasting behavior to the
when/then
pattern similar to Polars (see #2645) for our eager backends and dask.The implementation for the Lazy backends will require #2652 unless we want to introduce some hacky workarounds (e.g. we may be able to do this by joining after an aggregation to "broadcast" results; however I haven't fully thought through how this would look in the codebase).