-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
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?
Changes from 18 commits
bc9a942
558569f
bbbc720
e2f32cb
6fd8986
d2d5f62
475f2d1
db30b58
55fe420
cb94cf7
71e442e
b6bd3af
8bac997
89bc1b4
f154cf5
eed6121
9ac81f0
7e3fd3a
8c5ffff
5516517
2437ce2
9556aa4
b64b8a7
9574746
c073c0b
4eea08e
bbc5612
0851593
98fb602
915b8a7
7611f59
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -9701,6 +9701,7 @@ def _where( | |||||
# align the cond to same shape as myself | ||||||
cond = common.apply_if_callable(cond, self) | ||||||
if isinstance(cond, NDFrame): | ||||||
cond = cond.fillna(True) | ||||||
# CoW: Make sure reference is not kept alive | ||||||
if cond.ndim == 1 and self.ndim == 2: | ||||||
cond = cond._constructor_expanddim( | ||||||
|
@@ -9712,6 +9713,9 @@ def _where( | |||||
else: | ||||||
if not hasattr(cond, "shape"): | ||||||
cond = np.asanyarray(cond) | ||||||
else: | ||||||
cond = np.array(cond) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we avoid the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we might be able to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @mroeschke ,confirmed that all the checks passed. Could you review the code change? |
||||||
cond[isna(cond)] = True | ||||||
if cond.shape != self.shape: | ||||||
raise ValueError("Array conditional must be same shape as self") | ||||||
cond = self._constructor(cond, **self._construct_axes_dict(), copy=False) | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
import numpy as np | ||
import pytest | ||
|
||
import pandas.util._test_decorators as td | ||
|
||
from pandas import Series | ||
import pandas._testing as tm | ||
|
||
|
@@ -67,3 +69,19 @@ def test_mask_inplace(): | |
rs = s.copy() | ||
rs.mask(cond, -s, inplace=True) | ||
tm.assert_series_equal(rs, s.mask(cond, -s)) | ||
|
||
|
||
@pytest.mark.parametrize( | ||
"dtype", | ||
[ | ||
"Int64", | ||
pytest.param("int64[pyarrow]", marks=td.skip_if_no("pyarrow")), | ||
], | ||
) | ||
def test_mask_na(dtype): | ||
# We should not be filling pd.NA. See GH#60729 | ||
series = Series([None, 1, 2, None, 3, 4, None], dtype=dtype) | ||
result = series.mask(series <= 2, -99) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also test the case where the condition is an ndarray and a list. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @rhshadrach , add tests for a list and an ndarray. Could you review the code change? |
||
expected = Series([None, -99, -99, None, 3, 4, None], dtype=dtype) | ||
|
||
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.
Below we
fillna
using the value ofinplace
. UsingTrue
as done here looks correct, usinginplace
seems to be a bug. On main: