Replace with nested dict raises for overlapping keys
State: closed

- [x] closes #27660 
- [ ] tests added / passed
- [ ] passes `black pandas`
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: DataFrame
description: DataFrame data structure

Comments:
indeed, there are several ways to represent True/False as 1/0, and astype is one of them. But since based on documentation of replace, `{col_name: {replace dict}}` can be used to replace values, and should also work for this boolean case. However, the boolean case is not working, so I feel it is a bug and should fix. I just have not thought of a nice way instead of working around hash and would be very happy to hear nicer idea! @WillAyd 
Though it is interesting that this works currently:

```python
>>> df = pd.DataFrame({"col":[False, True]})
>>> df.replace({False:0, True:1})
   col
0    0
1    1
```

While the example from the OP doesn't

```python
>>> df.replace({"col": {False:0, True:1}})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/williamayd/clones/pandas/pandas/core/frame.py", line 4210, in replace
    method=method,
  File "/Users/williamayd/clones/pandas/pandas/core/generic.py", line 6645, in replace
    "Replacement not allowed with "
ValueError: Replacement not allowed with overlapping keys and values
```

@charlesdong1991 any idea how the path without column selection handles this?
@WillAyd I also noticed this and it's because in the case of `replace.{col_name: {replace dict}}`, there is a nested dictionary, and will cause `are_mappings` on the line of 6628 in `generic.py` containing a True, but in the case `replace({False:0, True:1})`, `any(are_mappings)` will be `False` and code will jump to line 6657 directly. I am very sorry i don't know how to copy paste the code here. Hope my description is clear enough ^^

@charlesdong1991 you can click on a line number in GH and share a permalink to make things easier:

<img width="539" alt="image" src="https://user-images.githubusercontent.com/609873/62378347-7b4cd500-b4f9-11e9-9b8e-58dab9b9d567.png">


Here's what I think you are referencing

https://github.com/pandas-dev/pandas/blob/1fa1ad91b29c5474cbb86cbcbcdcd50537cad0ae/pandas/core/generic.py#L6628
ahh, thanks very much!! Get it!

On Fri, 2 Aug 2019, 16:46 William Ayd <notifications@github.com> wrote:

> @charlesdong1991 <https://github.com/charlesdong1991> you can click on a
> line number in GH and share a permalink to make things easier:
>
> [image: image]
> <https://user-images.githubusercontent.com/609873/62378347-7b4cd500-b4f9-11e9-9b8e-58dab9b9d567.png>
>
> Here's what I think you are referencing
>
>
> https://github.com/pandas-dev/pandas/blob/1fa1ad91b29c5474cbb86cbcbcdcd50537cad0ae/pandas/core/generic.py#L6628
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/27696?email_source=notifications&email_token=ACGXEOAGOFLU2AEKXROTDUDQCRCFJA5CNFSM4IIRIILKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3N6ORY#issuecomment-517728071>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ACGXEOAD7PXMQFV7URFRL3LQCRCFJANCNFSM4IIRIILA>
> .
>

It looks like this behavior was intentionally added in #6429 quite a few years back. I'm not sure why the distinction is made for only *nested* dictionaries and maybe it's not valid any more (at least if it works with only a single dictionary) so certainly if you'd like to investigate and put forth a proposal would be appreciated

@charlesdong1991 you always seem to find these tricky PRs ha!
Yeah, indeed, I was also confused with the inconsistency last night when
reading through the code base. One proposal is adding same logic for
another and update docstring to let user know boolean doesn't work with
replace and should use other method like astype to convert boolean to
binary. Or also remove the above error check. Or keep as is and live with
it. But I believe either way will be controversial and seems buggy to some
corner case.

haha, honestly I really wanna avoid contributing to those tricky PRs and
make more and impactful PRs. Need to keep improving and learn to choose
less tricky PRs ^^


On Fri, 2 Aug 2019, 18:29 William Ayd <notifications@github.com> wrote:

> It looks like this behavior was intentionally added in #6429
> <https://github.com/pandas-dev/pandas/pull/6429> quite a few years back.
> I'm not sure why the distinction is made for only *nested* dictionaries
> and maybe it's not valid any more (at least if it works with only a single
> dictionary) so certainly if you'd like to investigate and put forth a
> proposal would be appreciated
>
> @charlesdong1991 <https://github.com/charlesdong1991> you always seem to
> find these tricky PRs ha!
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/27696?email_source=notifications&email_token=ACGXEOHV25IYEDZZTRCDBLLQCROIJA5CNFSM4IIRIILKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3OHK3I#issuecomment-517764461>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ACGXEOHOSWFFS7WOPHDJ4M3QCROIJANCNFSM4IIRIILA>
> .
>

These contributions are all really good. What happens if you remove the error check?
sorry, I will do a check after weekend, in the airport for a short trip
now.. wish you have a nice weekend :)


On Fri, 2 Aug 2019, 18:08 William Ayd <notifications@github.com> wrote:

> These contributions are all really good. What happens if you remove the
> error check?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/27696?email_source=notifications&email_token=ACGXEODURFGVRDQIP2EB23LQCRS2TA5CNFSM4IIRIILKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3OKNOY#issuecomment-517777083>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ACGXEOABQX2PPIOYB2OF3CDQCRS2TANCNFSM4IIRIILA>
> .
>

if removing the error check, everything will work as is. And test file in origin PR was moved to `pandas/tests/frame/test_replace.py` and those two tests will fail. I think it largely depends on which behavior people want to keep, but i do feel some consistency might be good to have. @WillAyd 
@charlesdong1991 can you update to use the consistent approach described above?
just to double check to ensure I undsertand correclty: preferable way is to remove the error check to avoid `replace({True: 1, False: 0})` to keep consistency of not supporting such replacement, right? @WillAyd 
Right - I don't think we should have that error check since it only applies to nested dictionaries when it works as is for a "simple" dictionary replacement
> Right - I don't think we should have that error check since it only applies to nested dictionaries when it works as is for a "simple" dictionary replacement

agree, thanks for your quick reply! @WillAyd appreciate a lot!
Hello @charlesdong1991! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:





There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2019-08-27 06:54:23 UTC
ping

sorry again for that unintentional push, now I think this PR is ready for review @WillAyd 
thanks @jschendel @WillAyd 

ping
thanks @charlesdong1991