Replace with nested dict raises for overlapping keys #27696

charlesdong1991 · 2019-08-01T14:26:59Z

closes Pandas replace does not work with booleans #27660
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

WillAyd

Having second thoughts about this change - not sure working around hashes is universally appropriate.

Instead of replace should users just be doing an astype(int) if they want to represent True / False as 1 and 0?

charlesdong1991 · 2019-08-01T17:46:02Z

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

jschendel

I also have the same thoughts/concerns as @WillAyd. I think the issue in question is an implementation detail that we might have to live with, as I'm not sure there's a clean fix that doesn't involve a substantial rewrite or a lot workarounds that hurt maintainability. I could be wrong about this but no clean solutions immediately come to mind.

jschendel · 2019-08-01T18:15:28Z

pandas/tests/generic/test_generic.py

+        # GH 27660
+        df = DataFrame({"col": [False, True, 0, 1]})
+
+        result = df.replace({"col": {False: 0, True: 1}})


If you were to switch the replacement to {False: 1, True: 0} then this test will fail as the integers will get incorrectly replaced:

In [2]: df = pd.DataFrame({'col': [False, True, 0, 1]}) In [3]: df.replace({'col': {False: 1, True: 0}}) Out[3]: col 0 1 1 0 2 1 3 0

I think the current version of the test is passing because 0/1 are just getting replaced by themselves.

This is a consequence of how Python handles hashing, specifically 0/False have the same hash and evaluate equally with == (Python's fallback on hash collision), so they'll be considered the same in set operations or when looking up keys in a dict (likewise for 1/True).

ehh, you are very right, it's not good solution @jschendel

jschendel · 2019-08-01T18:15:32Z

pandas/core/generic.py

+                    # add another check to avoid boolean being regarded
+                    # as binary in python set
+                    if set(keys) & set(values) and set(map(str, keys)) & set(
+                        map(str, values)


I think this might be a little bit too permissive now, as it will allow {0: 1.0, 1: 'a'}, which was previously rejected (might not actually matter but is a change in behavior we should be cognizant of).

yeah, i should have thought of it more thoroughly

Can this just not be removed altogether? Not clear on the purpose of it

WillAyd · 2019-08-01T23:17:37Z

Though it is interesting that this works currently:

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

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

charlesdong1991 · 2019-08-02T11:06:44Z

@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 ^^

WillAyd · 2019-08-02T14:45:58Z

@charlesdong1991 you can click on a line number in GH and share a permalink to make things easier:

Here's what I think you are referencing

pandas/pandas/core/generic.py

Line 6628 in 1fa1ad9

are_mappings = [is_dict_like(v) for v in values]

charlesdong1991 · 2019-08-02T15:03:52Z

ahh, thanks very much!! Get it!

…

On Fri, 2 Aug 2019, 16:46 William Ayd ***@***.***> 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 <#27696?email_source=notifications&email_token=ACGXEOAGOFLU2AEKXROTDUDQCRCFJA5CNFSM4IIRIILKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3N6ORY#issuecomment-517728071>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACGXEOAD7PXMQFV7URFRL3LQCRCFJANCNFSM4IIRIILA> .

WillAyd · 2019-08-02T16:28:55Z

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!

charlesdong1991 · 2019-08-02T17:05:37Z

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 ***@***.***> wrote: It looks like this behavior was intentionally added in #6429 <#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 <#27696?email_source=notifications&email_token=ACGXEOHV25IYEDZZTRCDBLLQCROIJA5CNFSM4IIRIILKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3OHK3I#issuecomment-517764461>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACGXEOHOSWFFS7WOPHDJ4M3QCROIJANCNFSM4IIRIILA> .

WillAyd · 2019-08-02T17:08:11Z

These contributions are all really good. What happens if you remove the error check?

charlesdong1991 · 2019-08-02T18:47:32Z

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 ***@***.***> 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 <#27696?email_source=notifications&email_token=ACGXEODURFGVRDQIP2EB23LQCRS2TA5CNFSM4IIRIILKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3OKNOY#issuecomment-517777083>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACGXEOABQX2PPIOYB2OF3CDQCRS2TANCNFSM4IIRIILA> .

charlesdong1991 · 2019-08-06T07:07:58Z

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

WillAyd · 2019-08-23T13:34:18Z

@charlesdong1991 can you update to use the consistent approach described above?

charlesdong1991 · 2019-08-23T14:07:49Z

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

WillAyd · 2019-08-23T14:10:01Z

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

charlesdong1991 · 2019-08-23T14:21:25Z

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!

pep8speaks · 2019-08-24T07:46:19Z

Hello @charlesdong1991! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

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

Comment last updated at 2019-08-27 06:54:23 UTC

WillAyd

Can you add a whatsnew for v1.0.0?

pandas/tests/generic/test_generic.py

WillAyd · 2019-08-24T08:00:31Z

pandas/core/generic.py

+                    # add another check to avoid boolean being regarded
+                    # as binary in python set
+                    if set(keys) & set(values) and set(map(str, keys)) & set(
+                        map(str, values)


Can this just not be removed altogether? Not clear on the purpose of it

charlesdong1991 · 2019-08-24T08:25:43Z

ping

sorry again for that unintentional push, now I think this PR is ready for review @WillAyd

WillAyd

Minor things but generally this looks good. Back over to you @jschendel

doc/source/whatsnew/v1.0.0.rst

pandas/tests/frame/test_replace.py

WillAyd

lgtm. @jschendel or whomever else wants to take a look merge if happy

jschendel

lgtm; a couple small comments since there's a merge conflict that needs to be resolved

pandas/tests/frame/test_replace.py

charlesdong1991 · 2019-08-27T07:30:20Z

thanks @jschendel @WillAyd

ping

jschendel · 2019-08-27T14:09:55Z

thanks @charlesdong1991

charlesdong1991 added 4 commits December 3, 2018 17:43

remove \n from docstring

7e461a1

fix conflicts

1314059

Merge remote-tracking branch 'upstream/master'

8bcb313

Fix boolean error in replace

4f4f293

WillAyd reviewed Aug 1, 2019

View reviewed changes

jschendel suggested changes Aug 1, 2019

View reviewed changes

tst

7529084

WillAyd added this to the 0.25.2 milestone Aug 23, 2019

WillAyd modified the milestones: 0.25.2, 1.0 Aug 24, 2019

Merge remote-tracking branch 'upstream/master' into fix_issue_27660

2c6d5f6

WillAyd requested changes Aug 24, 2019

View reviewed changes

code change based on review

eb6cee8

WillAyd requested changes Aug 25, 2019

View reviewed changes

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

pandas/tests/frame/test_replace.py Outdated Show resolved Hide resolved

pandas/tests/frame/test_replace.py Outdated Show resolved Hide resolved

WillAyd changed the title ~~Fix pandas replace does not work with boolean~~ Replace with nested dict raises for overlapping keys Aug 25, 2019

code change based on review

18bd517

WillAyd added the DataFrame DataFrame data structure label Aug 25, 2019

WillAyd approved these changes Aug 25, 2019

View reviewed changes

jschendel approved these changes Aug 26, 2019

View reviewed changes

pandas/tests/frame/test_replace.py Outdated Show resolved Hide resolved

pandas/tests/frame/test_replace.py Outdated Show resolved Hide resolved

charlesdong1991 added 2 commits August 27, 2019 08:52

fix conflict

20ae4f4

minor naming change

2c97495

jschendel merged commit 041b6b1 into pandas-dev:master Aug 27, 2019

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

Replace with nested dict raises for overlapping keys (pandas-dev#27696)

3fd0e2b

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

Replace with nested dict raises for overlapping keys (pandas-dev#27696)

d16b329

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace with nested dict raises for overlapping keys #27696

Replace with nested dict raises for overlapping keys #27696

charlesdong1991 commented Aug 1, 2019 •

edited by WillAyd

Loading

WillAyd left a comment

charlesdong1991 commented Aug 1, 2019 •

edited

Loading

jschendel left a comment

jschendel Aug 1, 2019

charlesdong1991 Aug 1, 2019

jschendel Aug 1, 2019

charlesdong1991 Aug 1, 2019

WillAyd Aug 24, 2019

WillAyd commented Aug 1, 2019

charlesdong1991 commented Aug 2, 2019

WillAyd commented Aug 2, 2019

charlesdong1991 commented Aug 2, 2019 via email

WillAyd commented Aug 2, 2019

charlesdong1991 commented Aug 2, 2019 via email

WillAyd commented Aug 2, 2019

charlesdong1991 commented Aug 2, 2019 via email

charlesdong1991 commented Aug 6, 2019

WillAyd commented Aug 23, 2019

charlesdong1991 commented Aug 23, 2019 •

edited

Loading

WillAyd commented Aug 23, 2019

charlesdong1991 commented Aug 23, 2019

pep8speaks commented Aug 24, 2019 •

edited

Loading

WillAyd left a comment

WillAyd Aug 24, 2019

charlesdong1991 commented Aug 24, 2019

WillAyd left a comment

WillAyd left a comment

jschendel left a comment

charlesdong1991 commented Aug 27, 2019

jschendel commented Aug 27, 2019

Replace with nested dict raises for overlapping keys #27696

Replace with nested dict raises for overlapping keys #27696

Conversation

charlesdong1991 commented Aug 1, 2019 • edited by WillAyd Loading

WillAyd left a comment

Choose a reason for hiding this comment

charlesdong1991 commented Aug 1, 2019 • edited Loading

jschendel left a comment

Choose a reason for hiding this comment

jschendel Aug 1, 2019

Choose a reason for hiding this comment

charlesdong1991 Aug 1, 2019

Choose a reason for hiding this comment

jschendel Aug 1, 2019

Choose a reason for hiding this comment

charlesdong1991 Aug 1, 2019

Choose a reason for hiding this comment

WillAyd Aug 24, 2019

Choose a reason for hiding this comment

WillAyd commented Aug 1, 2019

charlesdong1991 commented Aug 2, 2019

WillAyd commented Aug 2, 2019

charlesdong1991 commented Aug 2, 2019 via email

WillAyd commented Aug 2, 2019

charlesdong1991 commented Aug 2, 2019 via email

WillAyd commented Aug 2, 2019

charlesdong1991 commented Aug 2, 2019 via email

charlesdong1991 commented Aug 6, 2019

WillAyd commented Aug 23, 2019

charlesdong1991 commented Aug 23, 2019 • edited Loading

WillAyd commented Aug 23, 2019

charlesdong1991 commented Aug 23, 2019

pep8speaks commented Aug 24, 2019 • edited Loading

Comment last updated at 2019-08-27 06:54:23 UTC

WillAyd left a comment

Choose a reason for hiding this comment

WillAyd Aug 24, 2019

Choose a reason for hiding this comment

charlesdong1991 commented Aug 24, 2019

WillAyd left a comment

Choose a reason for hiding this comment

WillAyd left a comment

Choose a reason for hiding this comment

jschendel left a comment

Choose a reason for hiding this comment

charlesdong1991 commented Aug 27, 2019

jschendel commented Aug 27, 2019

charlesdong1991 commented Aug 1, 2019 •

edited by WillAyd

Loading

charlesdong1991 commented Aug 1, 2019 •

edited

Loading

charlesdong1991 commented Aug 23, 2019 •

edited

Loading

pep8speaks commented Aug 24, 2019 •

edited

Loading