-
-
Notifications
You must be signed in to change notification settings - Fork 18.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
BUG: Add fillna at the beginning of _where not to fill NA. #60729 #60772
base: main
Are you sure you want to change the base?
Conversation
|
||
def test_mask_na(): | ||
# We should not be filling pd.NA. See GH#60729 | ||
series = Series([None, 1, 2, None, 3, 4, None]) |
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 think this test can be simplified a bit to reduce the number of operations. Maybe something like:
ser = Series([None, 1, 2, None, 3, 4, None], dtype=pd.Int64Dtype())
result = ser.mask(ser <= 2, -99)
expected = Series([None, 1, 2, None, -99, -99, None], dtype=pd.Int64Dtype())
tm.assert_series_equal(result, expected)
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 the suggestion, just updated the code.
@@ -9674,6 +9674,13 @@ def _where( | |||
if axis is not None: | |||
axis = self._get_axis_number(axis) | |||
|
|||
# We should not be filling NA. See GH#60729 |
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.
Is this trying to fill missing values when NaN is the missing value indicator? I don't think that is right either - the missing values should propogate for all types. We may just be missing coverage for the NaN case (which should be added to the test)
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 the feedback, @WillAyd .
I thought we could make the values propagate by filling cond
with True
, since _where()
would finally keep the values in self
alive where its cond
is True
.
Even if I don't fill those values here, _where
would call fillna()
using inplace at the below code. That's also why the result varies depending on whether inpalce=True
or not.
Lines 9695 to 9698 in e3b2de8
# make sure we are boolean | |
fill_value = bool(inplace) | |
cond = cond.fillna(fill_value) | |
cond = cond.infer_objects() |
Could you explain in more detail what you mean by propagate for all type? Do you mean we need to keep NA as it is even after this line?
Co-authored-by: WillAyd <will_ayd@innobi.io>
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.Added fillna at the beginning of _where so that we can fill pd.NA.
Since this is my first PR, please correct me if I'm mistaken. Thanks!