ENH: Allow map with abc mapping
State: closed

- [x] closes #29733 (partly, look at my issue comment for details on why it won't exactly work on the issue's example)
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Enhancement
name: Algos
description: Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff
name: Series
description: Series data structure

Comments:
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 <notifications@github.com>
wrote:

> *@WillAyd* 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
> <https://github.com/pandas-dev/pandas/pull/29788#discussion_r349737661>:
>
> > @@ -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
> <https://github.com/pandas-dev/pandas/pull/29788#discussion_r349738912>:
>
> > @@ -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
> <https://github.com/pandas-dev/pandas/pull/29788?email_source=notifications&email_token=AAKAOIQADNZIIDNSMG5VKADQVARXPA5CNFSM4JQNDNTKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCMWUNVY#pullrequestreview-321734359>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAKAOIWCCMV5AXPYT46DJITQVARXPANCNFSM4JQNDNTA>
> .
>

> 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`.
lgtm. ping on green.
@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?
> lgtm. @ohad83 can you merge master one more time? I think should get CI green

@jreback @WillAyd It's green now

@ohad83 can you rebase and address Will's comment if still relevant
can you merge master and will look again
@ohad83 going to have this close the original issue. if the remaining case is worthile, then pls open a new issue.
@WillAyd if any comments
Thanks @ohad83 great PR