Adding test_map_missing_mixed to test_apply.py in pandas test suite series #20574

readyready15728 · 2018-04-02T04:22:32Z

Checklist for other PRs (remove this part if you are doing a PR for the pandas documentation sprint):

closes Inconsistent behavior of .map() #20495
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry (n/a)

…eries

…py in pandas test suite series

WillAyd

Based off of the comments in the linked issue I think there are (at least) three scenarios we wanted to test:

Mapping the NA value to a new value
Mapping a str value to a new value AND
Mapping an int value to a new value

You covered the first two but left out the third so we should add that.

With that said, whenever you have tests that are repetitive in nature and varying just slightly you should be thinking about parametrizing them. In this case, I think you should parametrize the test with values of "mapping,exp" where the former is the dict you want to use in the map function and the latter is the result you expect (should be a series in each case). Below is a relatively close example of how this works - take a look at that and see if you can apply something similar here

pandas/pandas/tests/groupby/test_groupby.py

Line 2063 in 4efb39f

def test_rank_resets_each_group(self, pct, exp):

readyready15728 · 2018-04-02T04:49:37Z

Alright, I think I have something here. Is there any convenient way for me to run just an individual test file rather than all tests in their entirety?

WillAyd · 2018-04-02T04:51:01Z

pytest path.to.the.module::ClassName::TestName would be the most granular, though you can strip off any of those elements to move up a level and run more tests

…suite series

pep8speaks · 2018-04-02T04:56:23Z

Hello @readyready15728! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on April 03, 2018 at 00:03 Hours UTC

codecov · 2018-04-02T04:56:46Z

Codecov Report

Merging #20574 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #20574      +/-   ##
==========================================
+ Coverage   91.82%   91.82%   +<.01%     
==========================================
  Files         152      153       +1     
  Lines       49255    49256       +1     
==========================================
+ Hits        45226    45227       +1     
  Misses       4029     4029

Flag	Coverage Δ
#multiple	`90.2% <ø> (ø)`	⬆️
#single	`41.9% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/resample.py	`96.43% <0%> (ø)`	⬆️
pandas/core/groupby.py
pandas/core/groupby/__init__.py	`100% <0%> (ø)`
pandas/core/groupby/groupby.py	`92.55% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4efb39f...e927be4. Read the comment docs.

…py in pandas test suite series (again)

readyready15728 · 2018-04-02T04:58:58Z

The parametrized test checks out.

WillAyd · 2018-04-02T05:02:47Z

pandas/tests/series/test_apply.py

+        ({'string': 'another string' }, pd.Series(['another string'])),
+        ({42: 'the answer'}, pd.Series(['the answer']))])
+    def test_map_missing_mixed(self, mapping, exp):
+        s = pd.Series(list(mapping.keys())[0])


Nice job on the parametrization but in the process of updating this line I think we are losing sight of what we are trying to test. Originally you were constructing this series from list('abcd') and appending an NA record. Now all you are doing is using the key of your dict as the series value, but that's not the same test.

Now that I'm thinking about it it's probably good to also add a "vals" parameter in front of the mapping. For the first two scenarios you can use list('abcd') as you had before and for the third use range(4). Then just make your first line s = pd.Series(vals + [np.nan])

OK, I'll give that a shot.

…n pandas test suite series

readyready15728 · 2018-04-02T05:17:54Z

Now I have something that passes but I'm not sure it's idiomatic.

readyready15728 · 2018-04-02T05:21:42Z

Actually there is at least one last problem: I am calling pd.Series a bunch of times when I can just put it around exp in the assertion step.

…ite series

WillAyd

Make sure you get the tests to pass locally before pushing to GitHub. These have some logical errors that you need to work through

WillAyd · 2018-04-02T05:30:53Z

pandas/tests/series/test_apply.py

+
+    @pytest.mark.parametrize("vals,mapping,exp", [
+        (list('abc'), {np.nan: 'not NaN'}, ['not NaN']),
+        (list('abc'), {'string': 'another string'}, ['another string']),


Your mapping should contain one of the values in the series, so use 'a' instead of 'string'

The tests did pass

Just not like they should have

WillAyd · 2018-04-02T05:31:30Z

pandas/tests/series/test_apply.py

+    @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'])])


Same comment as above, use 1 instead of 42. Just for consistency make the value numeric as well instead of 'the answer'

WillAyd · 2018-04-02T05:32:01Z

pandas/tests/series/test_apply.py

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


Don't use the mapping keys here. s = pd.Series(vals + [np.nan]) is all you need

WillAyd · 2018-04-02T05:35:13Z

pandas/tests/series/test_apply.py

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


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

…andas test suite series

readyready15728 · 2018-04-02T05:48:21Z

Alright, I just learned that supplying -s to pytest allows output from print() statements to be shown. With that knowledge I was able to check whether the values being generated made sense before committing this time. Sorry for jumping the gun.

WillAyd

Very minor edit but otherwise lgtm. CI should run and a core maintainer will be able to review thereafter for merge ability.

Thanks for trying your hand at your first PR!

WillAyd · 2018-04-02T05:56:27Z

pandas/tests/series/test_apply.py

+        (list('abc'), {'a': 'a letter'}, ['a letter'] + [np.nan] * 3),
+        (list(range(3)), {0: 42}, [42] + [np.nan] * 3)])
+    def test_map_missing_mixed(self, vals, mapping, exp):
+        s = pd.Series(vals + [np.nan])


Can you add # GH20495 as a comment on its own line right below the method definition?

Between def ... and s ...?

Yes - you can look at test_with_nested_series in the same module for reference

TomAugspurger

Just need the comment referencing the issue, then we're good.

… test suite series

readyready15728 · 2018-04-03T00:04:33Z

All set

TomAugspurger · 2018-04-03T19:22:17Z

Thanks @readyready15728 !

readyready15728 added 3 commits March 30, 2018 09:08

Adding test_map_missing_mixed to test_apply.py in pandas test suite s…

e3ae4a1

…eries

Fixing test_map_missing_mixed in test_apply.py in pandas test suite s…

02e48d5

…eries

Eliminating extra whitespace in test_map_missing_mixed in test_apply.…

c7402a6

…py in pandas test suite series

WillAyd requested changes Apr 2, 2018

View reviewed changes

Parametrizing test_map_missing_mixed in test_apply.py in pandas test …

6ba3f07

…suite series

Eliminating extra whitespace in test_map_missing_mixed in test_apply.…

50f12d7

…py in pandas test suite series (again)

WillAyd requested changes Apr 2, 2018

View reviewed changes

Refining parametrization in test_map_missing_mixed in test_apply.py i…

138d096

…n pandas test suite series

Refactoring test_map_missing_mixed in test_apply.py in pandas test su…

9a88dd2

…ite series

WillAyd requested changes Apr 2, 2018

View reviewed changes

Fixing logical errors in test_map_missing_mixed in test_apply.py in p…

60ef503

…andas test suite series

WillAyd requested changes Apr 2, 2018

View reviewed changes

TomAugspurger approved these changes Apr 2, 2018

View reviewed changes

Adding issue tag to test_map_missing_mixed in test_apply.py in pandas…

e927be4

… test suite series

TomAugspurger merged commit 85c7900 into pandas-dev:master Apr 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding test_map_missing_mixed to test_apply.py in pandas test suite series #20574

Adding test_map_missing_mixed to test_apply.py in pandas test suite series #20574

readyready15728 commented Apr 2, 2018 •

edited

Loading

WillAyd left a comment •

edited

Loading

readyready15728 commented Apr 2, 2018 •

edited

Loading

WillAyd commented Apr 2, 2018

pep8speaks commented Apr 2, 2018 •

edited

Loading

codecov bot commented Apr 2, 2018 •

edited

Loading

readyready15728 commented Apr 2, 2018

WillAyd Apr 2, 2018

readyready15728 Apr 2, 2018

readyready15728 commented Apr 2, 2018

readyready15728 commented Apr 2, 2018

WillAyd left a comment

WillAyd Apr 2, 2018

readyready15728 Apr 2, 2018

readyready15728 Apr 2, 2018

WillAyd Apr 2, 2018

WillAyd Apr 2, 2018

WillAyd Apr 2, 2018

readyready15728 commented Apr 2, 2018

WillAyd left a comment

WillAyd Apr 2, 2018

readyready15728 Apr 2, 2018

WillAyd Apr 2, 2018

TomAugspurger left a comment

readyready15728 commented Apr 3, 2018

TomAugspurger commented Apr 3, 2018

Adding test_map_missing_mixed to test_apply.py in pandas test suite series #20574

Adding test_map_missing_mixed to test_apply.py in pandas test suite series #20574

Conversation

readyready15728 commented Apr 2, 2018 • edited Loading

WillAyd left a comment • edited Loading

Choose a reason for hiding this comment

readyready15728 commented Apr 2, 2018 • edited Loading

WillAyd commented Apr 2, 2018

pep8speaks commented Apr 2, 2018 • edited Loading

Comment last updated on April 03, 2018 at 00:03 Hours UTC

codecov bot commented Apr 2, 2018 • edited Loading

Codecov Report

readyready15728 commented Apr 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

readyready15728 commented Apr 2, 2018

readyready15728 commented Apr 2, 2018

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

readyready15728 commented Apr 2, 2018

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger left a comment

Choose a reason for hiding this comment

readyready15728 commented Apr 3, 2018

TomAugspurger commented Apr 3, 2018

readyready15728 commented Apr 2, 2018 •

edited

Loading

WillAyd left a comment •

edited

Loading

readyready15728 commented Apr 2, 2018 •

edited

Loading

pep8speaks commented Apr 2, 2018 •

edited

Loading

codecov bot commented Apr 2, 2018 •

edited

Loading