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.

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
Changes from all commits
Commits
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
90 changes: 49 additions & 41 deletions pandas/tests/base/test_ops.py
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 @@ -694,58 +695,65 @@ 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

for null_obj in [np.nan, None]:
for orig in self.objs:
o = orig.copy()
klass = type(o)
obj = index_or_series_obj
if 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


# values will not be changed
fill_value = obj.values[0] if len(obj) > 0 else 0
result = obj.fillna(fill_value)
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
if isinstance(obj, Series) and len(obj) == 0:
# TODO: GH-32543
pytest.xfail("Shallow copy for empty Series is bugged")
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

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