Repository: pandas-dev/pandas
Pull request: Adding test_map_missing_mixed to test_apply.py in pandas test suite series
Checklist for other PRs (remove this part if you are doing a PR for the pandas documentation sprint):

- [x] closes #20495
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry (n/a)


Problem statement:
Inconsistent behavior of .map()
#### Code Sample, a copy-pastable example if possible

```python
pd.Series(list('abc') + [np.nan]).map({np.nan: 'bar'})
```

Result:

```
0   NaN
1   NaN
2   NaN
3   NaN
dtype: float64
```

Compare:

```python
pd.Series(list('abc') + [np.nan]).map({'a': 'foo', np.nan: 'bar'})
```

Result:

```
0    foo
1    NaN
2    NaN
3    bar
dtype: object
```

#### Problem description

If only `np.nan` is present in the mapping, then the associated mapping is not carried out. If another value is present, then it is carried out. Forgive me if this is intended behavior for some reason.

#### Expected Output

I'm not sure what is appropriate here.

#### Output of ``pd.show_versions()``

<details>
INSTALLED VERSIONS
------------------
commit: None
python: 3.5.2.final.0
python-bits: 32
OS: Linux
OS-release: 4.4.0-116-generic
machine: i686
processor: i686
byteorder: little
LC_ALL: None
LANG: en_US.UTF-8
LOCALE: en_US.UTF-8

pandas: 0.22.0
pytest: None
pip: 8.1.1
setuptools: 20.7.0
Cython: None
numpy: 1.11.0
scipy: 0.17.0
pyarrow: None
xarray: None
IPython: 6.2.1
sphinx: None
patsy: 0.5.0
dateutil: 2.4.2
pytz: 2014.10
blosc: None
bottleneck: None
tables: 3.2.2
numexpr: 2.4.3
feather: None
matplotlib: 1.5.1
openpyxl: None
xlrd: None
xlwt: None
xlsxwriter: None
lxml: None
bs4: 4.4.1
html5lib: 0.999
sqlalchemy: None
pymysql: None
psycopg2: None
jinja2: 2.10
s3fs: None
fastparquet: None
pandas_gbq: None
pandas_datareader: 0.6.0
</details>

Hints from issue discussion:
Strange but it looks like this works fine on master:

```python
In [3]: pd.Series(list('abc') + [np.nan]).map({np.nan: 'bar'})
Out[3]: 
0    NaN
1    NaN
2    NaN
3    bar
dtype: object
```

<details>

INSTALLED VERSIONS
------------------
commit: c36e9f705bdb2843398f6484dbda9458e38a7ce7
python: 3.6.4.final.0
python-bits: 64
OS: Darwin
OS-release: 17.4.0
machine: x86_64
processor: i386
byteorder: little
LC_ALL: None
LANG: en_US.UTF-8
LOCALE: en_US.UTF-8

pandas: 0.23.0.dev0+685.gc36e9f705
pytest: 3.4.1
pip: 9.0.1
setuptools: 38.5.1
Cython: 0.27.3
numpy: 1.14.1
scipy: 1.0.0
pyarrow: 0.8.0
xarray: 0.10.0
IPython: 6.2.1
sphinx: 1.7.0
patsy: 0.5.0
dateutil: 2.6.1
pytz: 2018.3
blosc: None
bottleneck: 1.2.1
tables: 3.4.2
numexpr: 2.6.4
feather: 0.4.0
matplotlib: 2.1.2
openpyxl: 2.5.0
xlrd: 1.1.0
xlwt: 1.3.0
xlsxwriter: 1.0.2
lxml: 4.1.1
bs4: 4.6.0
html5lib: 1.0.1
sqlalchemy: 1.2.1
pymysql: 0.8.0
psycopg2: None
jinja2: 2.10
s3fs: 0.1.3
fastparquet: 0.1.4
pandas_gbq: None
pandas_datareader: None
</details>
yeah this lgtm. can one of you check if we have a test covering this case, if not pls add (advise either way).
I looked through our tests but couldn't find anything to really cover this. From what I can tell there are a few similar tests in `pandas/tests/series/test_apply.py` but none of them mix NA and non-NA values in the caller.

@readyready15728 any interest in trying your hand at a PR to add this scenario? I think the module above would be a good location unless @jreback disagrees
@WillAyd

I barely know anything about pull requests. Could you be a little more specific about how I should proceed?
There is a pretty detailed guide for contributing to pandas - give that a look and let me know if you have any questions.

https://pandas.pydata.org/pandas-docs/stable/contributing.html
OK so do you intend that I submit a pull request to the code that implements .map() itself or the test code? And what is the intended behavior of .map()? I'm not sure which is right.
You should be adding a test to `pandas/tests/series/test_apply.py`. You can probably place it right below functions like `test_empty` and call it something like `test_missing_mixed`.

The expected value would be your second example from your original post
Alright, I'm virtually certain that my addition will work but I want to test it first. To do so, I need to get the tests to run. `pytest pandas` said that I need to do `python setup.py build_ext --inplace --force`. I did that but then I got more errors:

```
Exception: Cython-generated file 'pandas/_libs/reduction.c' not found.
                Cython is required to compile pandas from a development branch.
                Please install Cython or download a release package of pandas.
```

But I have Cython installed. :?

(Am running Ubuntu 16.04)

**EDIT:** I did some Googling and someone suggested using pip to get the latest version and now the Cython extensions are compiling.
There is something else about the behavior of `.map()` on my previous version (0.22) that may interest you:

```python
pd.Series([42] + [np.nan]).map({42: 'the answer', np.nan: 'not NaN'})
```

Has the result:

```
0    the answer
1           NaN
dtype: object
```

In other words, the intended effect doesn't happen when there's just an integer alongside `np.nan` in the map.  With strings the intended effect obviously happens and also with tuples but I didn't test floats and now I have the development version installed so going back will just be a pain. Might want to review 0.22 to see if any other bugs are lurking in there.

I am almost ready to do a pull request but I'd like a little guidance about creating a whatsnew entry, if it is needed at all.
Sorry for the delay in response. Since you aren't changing any of the core code (just adding tests) I don't think you need a whatsnew entry, since there's nothing that you can link to here that would change behavior.

That said, there's already a `map` change in the v0.23 whatsnew (see #15081) . If you are feeling ambitious, you could take a look at that and see if it is responsible for the fix here. If so, we can update that existing note to mention the bug being fixed and link to this issue as well

Commit message: Refactoring test_map_missing_mixed in test_apply.py in pandas test suite series

Patch to review:
diff --git a/pandas/tests/series/test_apply.py b/pandas/tests/series/test_apply.py
index 0780c846a6c19..93678ea4ad90a 100644
--- a/pandas/tests/series/test_apply.py
+++ b/pandas/tests/series/test_apply.py
@@ -576,3 +576,13 @@ def f(x):
         result = s.map(f)
         exp = pd.Series(['Asia/Tokyo'] * 25, name='XX')
         tm.assert_series_equal(result, exp)
+
+    @pytest.mark.parametrize("vals,mapping,exp", [
+        (list('abc'), {np.nan: 'not NaN'}, ['not NaN']),
+        (list('abc'), {'string': 'another string'}, ['another string']),
+        (list(range(3)), {42: 'the answer'}, ['the answer'])])
+    def test_map_missing_mixed(self, vals, mapping, exp):
+        s = pd.Series(vals + [list(mapping.keys())[0]])
+        result = s.map(mapping)
+
+        tm.assert_series_equal(result[-1:].reset_index(drop=True), pd.Series(exp))


Code review comments:
diff_hunk: @@ -576,3 +576,13 @@ def f(x):
         result = s.map(f)
         exp = pd.Series(['Asia/Tokyo'] * 25, name='XX')
         tm.assert_series_equal(result, exp)
+
+    @pytest.mark.parametrize("vals,mapping,exp", [
+        (list('abc'), {np.nan: 'not NaN'}, ['not NaN']),
+        (list('abc'), {'string': 'another string'}, ['another string']),
+        (list(range(3)), {42: 'the answer'}, ['the answer'])])
path: pandas/tests/series/test_apply.py
text: @user1:
Same comment as above, use 1 instead of 42. Just for consistency make the value numeric as well instead of 'the answer'

diff_hunk: @@ -576,3 +576,13 @@ def f(x):
         result = s.map(f)
         exp = pd.Series(['Asia/Tokyo'] * 25, name='XX')
         tm.assert_series_equal(result, exp)
+
+    @pytest.mark.parametrize("vals,mapping,exp", [
+        (list('abc'), {np.nan: 'not NaN'}, ['not NaN']),
+        (list('abc'), {'string': 'another string'}, ['another string']),
+        (list(range(3)), {42: 'the answer'}, ['the answer'])])
+    def test_map_missing_mixed(self, vals, mapping, exp):
+        s = pd.Series(vals + [list(mapping.keys())[0]])
+        result = s.map(mapping)
+
+        tm.assert_series_equal(result[-1:].reset_index(drop=True), pd.Series(exp))
path: pandas/tests/series/test_apply.py
text: @user1:
Think through the exp values you are passing in. They should obviously match the shape of your input but replace with NA values where appropriate.

For your first example, if you did `list('abc')` as your val `{'a': 'foo'}` as your mapping then your exp would be `['foo', np.nan, np.nan, np.nan]`.

I'm not sure what you are trying to do with `result[-1:].reset_index(drop=True)` but that's getting way too complicated. If you follow all of the above steps you can just do `tm.assert_series_equal(result, pd.Series(exp))`

diff_hunk: @@ -576,3 +576,13 @@ def f(x):
         result = s.map(f)
         exp = pd.Series(['Asia/Tokyo'] * 25, name='XX')
         tm.assert_series_equal(result, exp)
+
+    @pytest.mark.parametrize("vals,mapping,exp", [
+        (list('abc'), {np.nan: 'not NaN'}, ['not NaN']),
+        (list('abc'), {'string': 'another string'}, ['another string']),
+        (list(range(3)), {42: 'the answer'}, ['the answer'])])
+    def test_map_missing_mixed(self, vals, mapping, exp):
+        s = pd.Series(vals + [list(mapping.keys())[0]])
path: pandas/tests/series/test_apply.py
text: @user1:
Don't use the mapping keys here. `s = pd.Series(vals + [np.nan])` is all you need

diff_hunk: @@ -576,3 +576,13 @@ def f(x):
         result = s.map(f)
         exp = pd.Series(['Asia/Tokyo'] * 25, name='XX')
         tm.assert_series_equal(result, exp)
+
+    @pytest.mark.parametrize("vals,mapping,exp", [
+        (list('abc'), {np.nan: 'not NaN'}, ['not NaN']),
+        (list('abc'), {'string': 'another string'}, ['another string']),
path: pandas/tests/series/test_apply.py
text: @user1:
Your mapping should contain one of the values in the series, so use 'a' instead of 'string'

@author:
Riiight

@author:
The tests *did* pass

Just not like they should have