Skip to content

BUG: Series map ignoring na_action for dict or series mapper #47585

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 4 commits into from
Jul 21, 2022
Merged

BUG: Series map ignoring na_action for dict or series mapper #47585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1af0615
BUG: Series map ignoring na_action for dict or series mapper
phofl Jul 2, 2022
adc4ce4
Add tests
phofl Jul 9, 2022
43d5812
Merge remote-tracking branch 'upstream/main' into 47527
phofl Jul 9, 2022
8c3a2f2
Merge remote-tracking branch 'upstream/main' into 47527
phofl Jul 20, 2022
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
1 change: 1 addition & 0 deletions doc/source/whatsnew/v1.5.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -914,6 +914,7 @@ Missing
^^^^^^^
- Bug in :meth:`Series.fillna` and :meth:`DataFrame.fillna` with ``downcast`` keyword not being respected in some cases where there are no NA values present (:issue:`45423`)
- Bug in :meth:`Series.fillna` and :meth:`DataFrame.fillna` with :class:`IntervalDtype` and incompatible value raising instead of casting to a common (usually object) dtype (:issue:`45796`)
- Bug in :meth:`Series.map` not respecting ``na_action`` argument if mapper is a ``dict`` or :class:`Series` (:issue:`47527`)
- Bug in :meth:`DataFrame.interpolate` with object-dtype column not returning a copy with ``inplace=False`` (:issue:`45791`)
- Bug in :meth:`DataFrame.dropna` allows to set both ``how`` and ``thresh`` incompatible arguments (:issue:`46575`)
- Bug in :meth:`DataFrame.fillna` ignored ``axis`` when :class:`DataFrame` is single block (:issue:`47713`)
Expand Down
4 changes: 4 additions & 0 deletions pandas/core/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,10 @@ def _map_values(self, mapper, na_action=None):
f"{na_action} was passed"
)
raise ValueError(msg)

if na_action == "ignore":
mapper = mapper[mapper.index.notna()]
Copy link
Member

Choose a reason for hiding this comment

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

since there is quite a few paths through here, might be worth adding tests for categorical Series and also using a mapping that doesn't get converted to a series above.

I was quite happy to not treat this as a bug and just update the docs, see #46588 (comment) and #46588 (comment)

but it appears that @rhshadrach is happy to consider it a bug #47527 (comment)

so if we are honoring the na_action for types other than callables, it should probably be tested for the default dict case too.

Copy link
Member

Choose a reason for hiding this comment

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

oh! and change the docstring too.

without passing them to the mapping function -> without passing them to the mapping`

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm I would be ok with treating this ok as is, what do you think @rhshadrach ?

Copy link
Member

Choose a reason for hiding this comment

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

i'm also happy to treat it as a bug, but also consider it an enhancement so that we have a few more tests than a regular bugfix. (although we might already test)

I assume (not tried) that the default dict case goes through as a function and already honors the na_action and therefore no extra code needed. This also would be a good argument for treating it as a bug since the default dict case and regular dict are currently inconsistent?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm good point, but defaultdict seems to be off

mapping = defaultdict(int, {1: 10, np.nan: 42})
ser = Series([1, np.nan, 2])
result = ser.map(mapping)

This should return Series([10, 42, x])

where x is None? 2? Not sure, but this returns

Series([10, 0, 0])

Copy link
Member

Choose a reason for hiding this comment

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

I'm +1 on either option here - fixing the behavior to match the current docs or changing the docs as @simonjayhawkins has suggested. I'd lean toward the latter, but not strongly.

Copy link
Member

Choose a reason for hiding this comment

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

This should return Series([10, 42, x])

where x is None? 2? Not sure, but this returns

Series([10, 0, 0])

The second 0 (index 2) looks correct, but index 1 returning 0 too looks incorrect.

defaultdict(int, {1: 10, np.nan: 42})[2]
# 0
int()
# 0
defaultdict(int, {1: 10, np.nan: 42})[np.nan]
# 42

I'm +1 on either option here - fixing the behavior to match the current docs or changing the docs as @simonjayhawkins has suggested. I'd lean toward the latter, but not strongly.

Ignore for a minute the latent bug identified above, when I made those comments I didn't appreciate that a defaultdict would take the same path as a callable and hence already takes account of the na_action kwarg.

mapping = {1: 10, np.nan: 42}
print(Series([1, np.nan, 2]).map(mapping, na_action="ignore"))
# 0    10.0
# 1    42.0
# 2     NaN
# dtype: float64

mapping = defaultdict(int, {1: 10, np.nan: 42})
print(Series([1, np.nan, 2]).map(mapping, na_action="ignore"))
# 0    10.0
# 1     NaN
# 2     0.0
# dtype: float64

So for consistency, i'm now favoring the the first option. (i.e generally happy with this PR)

Copy link
Member

Choose a reason for hiding this comment

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

The issue with the first zero (at index 1) in #47585 (comment) appears to be due to using np.nan in a dictionary:

ser = Series([1, np.nan, 2])
print(id(ser._values[1]), id(np.nan))
# 139702525343536 139704452752816

Because one gets a view of np.nan, the ids are different and therefore lookup fails. This is "expected" behavior when working with np.nan in a dictionary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah this makes sense, thanks very much. I’ll add additional tests and then this should be ready I think


# Since values were input this means we came from either
# a dict or a series and mapper should be an index
if is_categorical_dtype(self.dtype):
Expand Down
28 changes: 28 additions & 0 deletions pandas/tests/apply/test_series_apply.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,34 @@ def test_map_dict_na_key():
tm.assert_series_equal(result, expected)


@pytest.mark.parametrize("arg_func", [dict, Series])
def test_map_dict_ignore_na(arg_func):
# GH#47527
mapping = arg_func({1: 10, np.nan: 42})
ser = Series([1, np.nan, 2])
result = ser.map(mapping, na_action="ignore")
expected = Series([10, np.nan, np.nan])
tm.assert_series_equal(result, expected)


def test_map_defaultdict_ignore_na():
# GH#47527
mapping = defaultdict(int, {1: 10, np.nan: 42})
ser = Series([1, np.nan, 2])
result = ser.map(mapping)
expected = Series([10, 0, 0])
tm.assert_series_equal(result, expected)


def test_map_categorical_na_ignore():
# GH#47527
values = pd.Categorical([1, np.nan, 2], categories=[10, 1])
ser = Series(values)
result = ser.map({1: 10, np.nan: 42})
expected = Series([10, np.nan, np.nan])
tm.assert_series_equal(result, expected)


def test_map_dict_subclass_with_missing():
"""
Test Series.map with a dictionary subclass that defines __missing__,
Expand Down