Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: add when-then-otherwise expression #588
feat: add when-then-otherwise expression #588
Changes from 62 commits
d7446f4
6ebc78b
a3fdcc5
1ad1c94
55f394f
93e7121
f3770b7
cf92f80
a7f442a
7f23f05
7cc3aad
ab85e40
4a8ac56
8283f24
f1c667e
74937ea
5b030d6
7390e1a
279e3ad
63048ee
e96af89
d4f0e9c
99d9899
71e542d
8b1355a
eb36164
c9b09bf
2b1eabc
2ef564e
0e4773d
151fe14
add7b89
504c4ea
fd21c78
2f03bd0
37cc634
0454ac4
4ad28b7
0ded393
3280a3c
5c6deed
27d17d9
8688491
81039bf
1196fab
beba175
606feed
45684d4
21e3384
e2e0b92
ee2d9ba
e7f31bf
c79a24c
f1db4c4
06a3a97
bba70ea
b02906d
561856f
e21243a
ada9588
ead6649
bc129ee
ba3d158
7cbc759
28f55fb
47d6a35
ee835c2
08e689a
d8bc8b7
affcd7f
3b085cf
1983a94
a2c6661
0201428
e6d6117
dbf5b6e
fde2e9a
3b37c5a
6f0db73
8efb260
d738400
0599200
a90fbde
e9b7b28
3bb2629
733effc
eb81758
9654147
2bc5cc0
4b8a238
54d34d7
de4abfa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 would create an
object
dtype series, which we need to avoidthe dtype of
value_series
needs to be preservedThere 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.
I assume we are talking about the
None
possibility forotherwise_series
?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.
yup!
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.
ok, looking deeper into this
we have found an issue with
zip_with
in thepandas
implementation as in thepolars
documentation ofzip_with
, here,other
series already has to be of the same type of the series it is zipped with. No such guarantees are made in thepandas
implementation in narwhals, which leads to this issue.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.
I have added a fix for this in this pr for now, if you want we can move this fix into a separate pr
The fix is not entirely complete as of now, as it converts integer types to float types, where as the correct behaviour would be to convert them to nullable int types
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.
Now the functionality is as follows
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 for looking into this so deep!
i'd like to spend a little more time on this - nullable dtypes aren't generally supported by the data ecosystem unfortunately, so i'm not sure we should convert to them on behalf of the user
we'll get this in πͺ let's just take our time to avoid having to make breaking changes later
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.
Oh, definitely, let's make it work properly first and make sure to say a big no to breaking changes later on
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.
I had other thoughts on how to solve this for the type issue, which again mean changing the type of the column in pandas
float
typeinteger
caseThere 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.
Also note that the
when
expression is creating a brand new series and not overriding the original one