BUG: Series map ignoring na_action for dict or series mapper

[phofl]

• closes Series.map and ignore_na depends on mapping type #47527 (Replace xxxx with the Github issue number)
• Tests added and passed if fixing a bug or adding a new feature
• All code checks passed.
• Added type annotations to new arguments/methods/functions.
• Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

[simonjayhawkins]

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.

[simonjayhawkins]

oh! and change the docstring too.

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

[phofl]

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

[simonjayhawkins]

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?

[phofl]

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])

[rhshadrach]

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.

[simonjayhawkins]

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)

[rhshadrach]

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.

[phofl]

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

[jreback]

lgtm

[rhshadrach]

lgtm

[phofl]

Please don’t merge yet, this needs more tests

[rhshadrach]

@phofl - if you convert to draft, it can't be merged.

[phofl]

Tests added, ready now

[mroeschke]

Thanks @phofl