ENH: Allow map with abc mapping #29788

ohad83 · 2019-11-22T07:58:44Z

closes Series.map(m: Mapping) can fail with "object is not callable" #29733 (partly, look at my issue comment for details on why it won't exactly work on the issue's example)
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

doc/source/whatsnew/v1.0.0.rst

pandas/core/series.py

pandas/tests/series/test_apply.py

pandas/core/base.py

WillAyd

Looks good generally. cc @TomAugspurger and @simonjayhawkins for related discussions that we've had on types - not sure if this is something we would need to review holistically

pandas/core/base.py

pandas/core/series.py

TomAugspurger · 2019-11-22T20:16:59Z

Changes here look sensible. It's not immediately clear to my why `Series.__init__` had to change. Can we add a test in pandas/tests/series/test_constructors.py that shows the change? We may want to move these Mapping subclass test helpers to a central place and reuse them.

…

On Fri, Nov 22, 2019 at 12:41 PM William Ayd ***@***.***> wrote: ***@***.**** requested changes on this pull request. Looks good generally. cc @TomAugspurger <https://github.com/TomAugspurger> and @simonjayhawkins <https://github.com/simonjayhawkins> for related discussions that we've had on types - not sure if this is something we would need to review holistically ------------------------------ In pandas/core/base.py <#29788 (comment)>: > @@ -1215,8 +1215,8 @@ def _map_values(self, mapper, na_action=None): # we can fastpath dict/Series to an efficient map # as we know that we are not going to have to yield # python types - if isinstance(mapper, dict): - if hasattr(mapper, "__missing__"): + if isinstance(mapper, abc.Mapping): + if isinstance(mapper, dict) and hasattr(mapper, "__missing__"): I think can just simplify to isinstance(mapper, dict) then no? Check for *missing* after that is duplicative? ------------------------------ In pandas/core/series.py <#29788 (comment)>: > @@ -253,7 +254,7 @@ def __init__( else: data = data.reindex(index, copy=copy) data = data._data - elif isinstance(data, dict): + elif isinstance(data, abc.Mapping): Is this hit in a test case? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#29788?email_source=notifications&email_token=AAKAOIQADNZIIDNSMG5VKADQVARXPA5CNFSM4JQNDNTKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCMWUNVY#pullrequestreview-321734359>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKAOIWCCMV5AXPYT46DJITQVARXPANCNFSM4JQNDNTA> .

ohad83 · 2019-11-23T09:45:49Z

Changes here look sensible. It's not immediately clear to my why Series.__init__ had to change. Can we add a test in pandas/tests/series/test_constructors.py that shows the change? We may want to move these Mapping subclass test helpers to a central place and reuse them.

Added a test for constructing a Series with NonDictMapping and moved the NonDictMapping test subclass to pandas/util/testing.py.

pandas/core/base.py

pandas/core/series.py

jreback · 2019-12-08T18:09:07Z

pandas/core/series.py

@@ -253,7 +254,7 @@ def __init__(
                else:
                    data = data.reindex(index, copy=copy)
                data = data._data
-            elif isinstance(data, dict):
+            elif isinstance(data, abc.Mapping):


you can use is_dict_like here instead

jreback · 2019-12-08T18:09:17Z

pandas/core/base.py

@@ -1215,8 +1215,8 @@ def _map_values(self, mapper, na_action=None):
        # we can fastpath dict/Series to an efficient map
        # as we know that we are not going to have to yield
        # python types
-        if isinstance(mapper, dict):
-            if hasattr(mapper, "__missing__"):
+        if isinstance(mapper, abc.Mapping):


use is_dict_like

jreback · 2019-12-08T20:33:46Z

lgtm. ping on green.

WillAyd

lgtm. @ohad83 can you merge master one more time? I think should get CI green

ohad83 · 2019-12-09T21:08:25Z

@WillAyd Merged but still a test seems to fail, I am not really sure what's happening and how, if at all, it's related to this PR.. Any ideas?

ohad83 · 2019-12-14T10:46:44Z

lgtm. @ohad83 can you merge master one more time? I think should get CI green

@jreback @WillAyd It's green now

WillAyd · 2019-12-16T19:36:38Z

pandas/util/testing.py

@@ -2121,6 +2121,20 @@ def __init__(self, *args, **kwargs):
        dict.__init__(self, *args, **kwargs)


+class TestNonDictMapping(abc.Mapping):


Can you make this a fixture in pandas/tests/series/conftest.py instead of putting in pandas.util.testing? With the latter sometimes things can leak into our API and definitely wouldn't want that here

jbrockmendel · 2019-12-21T03:28:25Z

@ohad83 can you rebase and address Will's comment if still relevant

jreback · 2019-12-27T19:57:50Z

can you merge master and will look again

jreback · 2020-01-01T16:54:26Z

@ohad83 going to have this close the original issue. if the remaining case is worthile, then pls open a new issue.

Series.map

subclass. TST - Add a test for the aforementioned bug

DOC - Moved whatsnew of the aforementioned change from BUG to ENH

DOC - Added issue number for tests of Series.map with abc.Mapping argument

TST - Added a test of constructing a series from NonDictMapping

DOC - Typo fix

This reverts commit 5fe4140. Apparently it's an unwanted pattern checked for by code_checks.sh (from collections.abc import)

jreback · 2020-01-01T17:43:34Z

@WillAyd if any comments

WillAyd · 2020-01-02T01:35:39Z

Thanks @ohad83 great PR

ohad83 changed the title ~~Bug allow map with abc mapping~~ BUG - allow map with abc mapping Nov 22, 2019

ohad83 added a commit to ohad83/pandas that referenced this pull request Nov 22, 2019

DOC - Add bug of PR pandas-dev#29788 to whatsnew

9d534e0

gfyoung added Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff Series Series data structure Enhancement labels Nov 22, 2019

gfyoung changed the title ~~BUG - allow map with abc mapping~~ ENH: Allow map with abc mapping Nov 22, 2019

gfyoung reviewed Nov 22, 2019

View reviewed changes

doc/source/whatsnew/v1.0.0.rst Outdated Show resolved Hide resolved

ohad83 force-pushed the BUG-allow-map-with-abc-mapping branch from 7df89d3 to dadb76a Compare November 22, 2019 12:47

jreback requested changes Nov 22, 2019

View reviewed changes

pandas/core/series.py Outdated Show resolved Hide resolved

pandas/core/series.py Outdated Show resolved Hide resolved

pandas/tests/series/test_apply.py Outdated Show resolved Hide resolved

pandas/core/base.py Show resolved Hide resolved

ohad83 marked this pull request as ready for review November 22, 2019 13:20

ohad83 requested a review from jreback November 22, 2019 14:25

WillAyd requested changes Nov 22, 2019

View reviewed changes

pandas/core/base.py Show resolved Hide resolved

pandas/core/series.py Outdated Show resolved Hide resolved

ohad83 requested a review from WillAyd November 24, 2019 21:30

simonjayhawkins reviewed Nov 25, 2019

View reviewed changes

pandas/core/base.py Outdated Show resolved Hide resolved

pandas/core/series.py Outdated Show resolved Hide resolved

pandas/core/series.py Outdated Show resolved Hide resolved

ohad83 requested review from gfyoung and simonjayhawkins December 7, 2019 20:13

jreback requested changes Dec 8, 2019

View reviewed changes

jreback added this to the 1.0 milestone Dec 8, 2019

jreback approved these changes Dec 8, 2019

View reviewed changes

WillAyd approved these changes Dec 9, 2019

View reviewed changes

ohad83 force-pushed the BUG-allow-map-with-abc-mapping branch from 55a1dfa to 5c6bb3c Compare December 14, 2019 10:03

WillAyd requested changes Dec 16, 2019

View reviewed changes

ohad83 and others added 16 commits January 1, 2020 12:11

BUG - Allow a mapper of type collections.abc.Mapping when using

8bbe838

Series.map

BUG - Fixed Series.map to work with no-missing ABC Mapping

a5be925

TST - Added a test for Series.map with dict-like (abc.Mapping) mapper

e9d772a

BUG - In Series.map, only look for __missing__ if the mapper is a dict

1aeb197

subclass. TST - Add a test for the aforementioned bug

CLN - Add a newline for readability

e866095

CLN - Import sorting fix

3c00d57

DOC - Added collections.abc.Mapping to arg doc of Series.map

64df513

DOC - Moved whatsnew of the aforementioned change from BUG to ENH

DOC - Removed superfluous 'dict' from Series.map argument description

d823d5d

DOC - Added issue number for tests of Series.map with abc.Mapping argument

TST - Moved NonDictMapping test util class to pandas/util/testing.py

fa83960

TST - Added a test of constructing a series from NonDictMapping

CLN - Better way to import from collections.abc

e536b4d

DOC - Typo fix

Revert "CLN - Better way to import from collections.abc"

102b82a

This reverts commit 5fe4140. Apparently it's an unwanted pattern checked for by code_checks.sh (from collections.abc import)

DOC - Type fix

2cb36bb

CLN - remove example.feather which was mistakenly added

3074c8c

CLN: Checking is_dict_like instead of is abc.Mapping

49637e1

CLN: Changed import sorting

08403e9

use fixtures

97d245d

jreback force-pushed the BUG-allow-map-with-abc-mapping branch from 1f7d650 to 97d245d Compare January 1, 2020 17:13

jreback added 3 commits January 1, 2020 12:14

removed uneeded import

5dcb9a6

moar uneeded imports

8d3f279

missing import

f8eebc2

WillAyd approved these changes Jan 2, 2020

View reviewed changes

WillAyd merged commit d47c5a2 into pandas-dev:master Jan 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Allow map with abc mapping #29788

ENH: Allow map with abc mapping #29788

ohad83 commented Nov 22, 2019 •

edited by jreback

Loading

WillAyd left a comment

TomAugspurger commented Nov 22, 2019 via email

ohad83 commented Nov 23, 2019

jreback Dec 8, 2019

jreback Dec 8, 2019

jreback commented Dec 8, 2019

WillAyd left a comment

ohad83 commented Dec 9, 2019

ohad83 commented Dec 14, 2019

WillAyd Dec 16, 2019

jbrockmendel commented Dec 21, 2019

jreback commented Dec 27, 2019

jreback commented Jan 1, 2020

jreback commented Jan 1, 2020

WillAyd commented Jan 2, 2020

		@@ -2121,6 +2121,20 @@ def __init__(self, args, *kwargs):
		dict.__init__(self, args, *kwargs)


		class TestNonDictMapping(abc.Mapping):

ENH: Allow map with abc mapping #29788

ENH: Allow map with abc mapping #29788

Conversation

ohad83 commented Nov 22, 2019 • edited by jreback Loading

WillAyd left a comment

Choose a reason for hiding this comment

TomAugspurger commented Nov 22, 2019 via email

ohad83 commented Nov 23, 2019

jreback Dec 8, 2019

Choose a reason for hiding this comment

jreback Dec 8, 2019

Choose a reason for hiding this comment

jreback commented Dec 8, 2019

WillAyd left a comment

Choose a reason for hiding this comment

ohad83 commented Dec 9, 2019

ohad83 commented Dec 14, 2019

WillAyd Dec 16, 2019

Choose a reason for hiding this comment

jbrockmendel commented Dec 21, 2019

jreback commented Dec 27, 2019

jreback commented Jan 1, 2020

jreback commented Jan 1, 2020

WillAyd commented Jan 2, 2020

ohad83 commented Nov 22, 2019 •

edited by jreback

Loading