Skip to content

CLN: Split and fixturized test_fillna in tests/base/test_ops.py #32483

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Mar 11, 2020
Merged

CLN: Split and fixturized test_fillna in tests/base/test_ops.py #32483

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
54630a8
split and fixturized test_fillna in tests/base/test_ops.py
SaturnFromTitan Mar 6, 2020
afe6e84
adjusting isinstance checks to use the ABC versions
SaturnFromTitan Mar 6, 2020
343dd67
reverted the isinstance checks for ABCIndex
SaturnFromTitan Mar 6, 2020
0a0bdfa
test_fillna is executed for empty objs as well now
SaturnFromTitan Mar 8, 2020
bb3eff6
Merge branch 'master' into split-and-fixturize-test_fillna
SaturnFromTitan Mar 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 46 additions & 40 deletions pandas/tests/base/test_ops.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
is_object_dtype,
needs_i8_conversion,
)
from pandas.core.dtypes.generic import ABCMultiIndex

import pandas as pd
from pandas import (
Expand Down Expand Up @@ -719,58 +720,63 @@ def test_drop_duplicates_series_vs_dataframe(self):
dropped_series = df[column].drop_duplicates(keep=keep)
tm.assert_frame_equal(dropped_frame, dropped_series.to_frame())

def test_fillna(self):
def test_fillna(self, index_or_series_obj):
# # GH 11343
# though Index.fillna and Series.fillna has separate impl,
# test here to confirm these works as the same

for orig in self.objs:

o = orig.copy()
values = o.values

# values will not be changed
result = o.fillna(o.astype(object).values[0])
if isinstance(o, Index):
tm.assert_index_equal(o, result)
else:
tm.assert_series_equal(o, result)
# check shallow_copied
assert o is not result
obj = index_or_series_obj
if len(obj) == 0:
pytest.skip("Test doesn't make sense on empty data")
elif isinstance(obj, ABCMultiIndex):
pytest.skip("MultiIndex doesn't support isna")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an open issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment below, this is related to MultiIndex not being able to hold "pure" NA values


for null_obj in [np.nan, None]:
for orig in self.objs:
o = orig.copy()
klass = type(o)
# values will not be changed
result = obj.fillna(obj.values[0])
if isinstance(obj, Index):
tm.assert_index_equal(obj, result)
else:
tm.assert_series_equal(obj, result)

if not allow_na_ops(o):
continue
# check shallow_copied
assert obj is not result

if needs_i8_conversion(o):
@pytest.mark.parametrize("null_obj", [np.nan, None])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using nulls_fixture should be taken care of in #32437

def test_fillna_null(self, null_obj, index_or_series_obj):
# # GH 11343
# though Index.fillna and Series.fillna has separate impl,
# test here to confirm these works as the same
obj = index_or_series_obj
klass = type(obj)

values = o.astype(object).values
fill_value = values[0]
values[0:2] = pd.NaT
else:
values = o.values.copy()
fill_value = o.values[0]
values[0:2] = null_obj
if not allow_na_ops(obj):
pytest.skip(f"{klass} doesn't allow for NA operations")
elif len(obj) < 1:
pytest.skip("Test doesn't make sense on empty data")
elif isinstance(obj, ABCMultiIndex):
pytest.skip(f"MultiIndex can't hold '{null_obj}'")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another open issue?

Copy link
Contributor Author

@SaturnFromTitan SaturnFromTitan Mar 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this by design? A MultiIndex must hold values on multiple levels, so its elements are tuples. Therefore a MultiIndex can't just hold a raw np.nan or None as a value. Maybe a tuple of nans might make sense, but I don't think that would be recognised as "na" by fillna.


expected = [fill_value] * 2 + list(values[2:])
values = obj.values
fill_value = values[0]
expected = values.copy()
if needs_i8_conversion(obj):
values[0:2] = iNaT
expected[0:2] = fill_value
else:
values[0:2] = null_obj
expected[0:2] = fill_value

expected = klass(expected, dtype=orig.dtype)
o = klass(values)
expected = klass(expected)
obj = klass(values)

# check values has the same dtype as the original
assert o.dtype == orig.dtype
result = obj.fillna(fill_value)
if isinstance(obj, Index):
tm.assert_index_equal(result, expected)
else:
tm.assert_series_equal(result, expected)

result = o.fillna(fill_value)
if isinstance(o, Index):
tm.assert_index_equal(result, expected)
else:
tm.assert_series_equal(result, expected)
# check shallow_copied
assert o is not result
# check shallow_copied
assert obj is not result

@pytest.mark.skipif(PYPY, reason="not relevant for PyPy")
def test_memory_usage(self, index_or_series_obj):
Expand Down