BUG: Fix behavior of replace_list with mixed types. #40555

hasan-yaman · 2021-03-21T15:15:44Z

closes BUG: Change in behavior of replace with integer series and float to_replace #40371
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

mzeitlin11

Thanks for the pr, @hasan-yaman!

pandas/tests/series/methods/test_replace.py

mzeitlin11 · 2021-03-21T15:51:41Z

Looks like this caused some test errors elsewhere: https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=56659&view=logs&j=b516280a-8c75-541b-8ba0-ae13561718a7&t=a9217cb1-24f6-5625-717d-72085a70e1d3&l=41

dsaxton · 2021-03-21T15:56:39Z

pandas/tests/frame/methods/test_replace.py

+    @pytest.mark.parametrize(
+        "s, to_replace, value, expected",
+        [
+            (DataFrame([1]), np.array([1.0]), [0], DataFrame([0])),


May want to test this for different container types for to_replace rather than only np.array

I added lists too. Should I add more types?

hasan-yaman · 2021-03-21T19:25:19Z

Thanks for the comments.

I reverted changes in cast.py file since it turns out making changes in cast.py wasn't good idea.
I updated the tests.

I need some help for the PR.

pandas/pandas/core/internals/blocks.py

Lines 801 to 804 in 9c4c821

    
           # Exclude anything that we know we won't contain 
        
           pairs = [ 
        
               (x, y) for x, y in zip(src_list, dest_list) if self._can_hold_element(x) 
        
           ]

Removing if statement here will solve the problem but i am wondering is there are better way to solve it?

jbrockmendel · 2021-03-22T16:58:42Z

pandas/tests/frame/methods/test_replace.py

@@ -1659,3 +1658,25 @@ def test_replace_bytes(self, frame_or_series):
        expected = obj.copy()
        obj = obj.replace({None: np.nan})
        tm.assert_equal(obj, expected)
+
+    @pytest.mark.parametrize(
+        "s, to_replace, value, expected",


can you give this a multi-letter name instead of 's'

pandas/tests/frame/methods/test_replace.py

simonjayhawkins · 2021-04-12T11:26:58Z

Thanks @hasan-yaman for the PR. can you fix-up failing tests... it appears actual fix has been lost in a subsequent commit and add a release note (will be 1.2.5 but can add to doc\source\whatsnew\v1.2.4.rst in the first instance and move once doc\source\whatsnew\v1.2.5.rst is available)

jreback · 2021-04-12T13:06:07Z

Thanks @hasan-yaman for the PR. can you fix-up failing tests... it appears actual fix has been lost in a subsequent commit and add a release note (will be 1.2.5 but can add to doc\source\whatsnew\v1.2.4.rst in the first instance and move once doc\source\whatsnew\v1.2.5.rst is available)

this is just a tests, no whatsnew needed.

simonjayhawkins · 2021-04-12T13:10:42Z

this is just a tests, no whatsnew needed.

IIUC this PR a regression fix for #40371 but the fix, added in the first commit, has been lost.

hasan-yaman · 2021-04-12T17:26:07Z

Actually, first commit was causing too many problems with the tests, I couldn't fix them. So, I reverted the changes from the first commit.
I am open to any idea to fix the problem. (Maybe others who know pandas better than me can attempt the fix the problem with separate PR.)

simonjayhawkins · 2021-04-12T17:57:40Z

I am open to any idea to fix the problem. (Maybe others who know pandas better than me can attempt the fix the problem with separate PR.)

The regression was caused by #38097, so the fix probably involves code changed in that PR. cc @jbrockmendel

jreback · 2021-04-13T14:19:12Z

this is just a tests, no whatsnew needed.

IIUC this PR a regression fix for #40371 but the fix, added in the first commit, has been lost.

what do you mean? if this doesn't have a patch then the tests shouldn't pass (or is this *already) fixed?

simonjayhawkins · 2021-04-13T14:23:00Z

the regression is not fixed.

github-actions · 2021-05-14T00:08:10Z

This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.

simonjayhawkins · 2021-05-24T12:31:19Z

this is just a tests, no whatsnew needed.

IIUC this PR a regression fix for #40371 but the fix, added in the first commit, has been lost.

what do you mean? if this doesn't have a patch then the tests shouldn't pass (or is this *already) fixed?

The tests are not passing.

simonjayhawkins · 2021-05-24T12:39:11Z

to_replace is dispatched to _replace_list if is_list_like(to_replace)

_replace_list only accepts lists, but also need to make sure that the list is python floats

>>> import numpy as np
>>> 
>>> type(np.array([1.0])[0])
<class 'numpy.float64'>
>>> 
>>> type(list(np.array([1.0]))[0])
<class 'numpy.float64'>
>>> 
>>> type(np.array([1.0]).tolist()[0])
<class 'float'>
>>>

or change _can_hold_element

simonjayhawkins · 2021-05-24T13:02:28Z

or change _can_hold_element

this maybe a better option since this is where we have the inconsistency

>>> 
>>> pd.core.dtypes.cast.can_hold_element(arr=pd.Series([1]).values, element=1.0)
True
>>> 
>>> pd.core.dtypes.cast.can_hold_element(
...     arr=pd.Series([1]).values, element=np.array([1.0])[0]
... )
False
>>>

simonjayhawkins · 2021-05-24T13:20:30Z

or change _can_hold_element

this maybe a better option since this is where we have the inconsistency

changed my mind again! since we have a mature backport branch and may not have any more patch releases on 1.2.x let's not make this change and just patch the code that was added in #38097 to convert a numpy array to a list of python objects.

…th-integers

lbittarello · 2021-05-24T18:34:53Z

pandas/tests/frame/methods/test_replace.py

+            ([1.0], [1], [0], [0.0]),
+        ],
+    )
+    @pytest.mark.parametrize("box", [list, tuple, np.array])


Would it be possible to test pd.array too? Better safe than sorry. :)

That would fail. EA is not a documented allowed type for the to_replace argument of the replace method. to be fair, nor is numpy array or tuple. The safest way to restore the old undocumented behavior is to revert #38097, but not sure that a performance hit for users passing the correct type is the way forward.

Any container, even a list, with numpy scalars will still fail. This is a fix for a specific case for an arguably incorrect usage without a perf hit for correct usage.

simonjayhawkins · 2021-05-26T14:40:39Z

@jreback can you take a look. not the best fix, but will be fixed properly ( I think) in #41644, which would be updated to remove the code added here on master straight after this is backported.

simonjayhawkins · 2021-06-01T11:15:53Z

pandas/core/internals/blocks.py

+        # until the issue is fixed properly in can_hold_element
+
+        # error: "Iterable[Any]" has no attribute "tolist"
+        if hasattr(src_list, "tolist"):


I could maybe change this to a simple isinstance(src_list, np.array)

simonjayhawkins · 2021-06-01T11:16:31Z

will merge (and backport) this shortly if no further comments

…th-integers

jreback · 2021-06-01T11:44:03Z

yep looks ok (rebase if u can though)

simonjayhawkins · 2021-06-01T12:23:37Z

yep looks ok (rebase if u can though)

merged master an hour ago, no commits to master since then

simonjayhawkins · 2021-06-01T12:36:49Z

Thanks @hasan-yaman

…mixed types.

…s. (#41761) Co-authored-by: hasan-yaman <[email protected]>

Co-authored-by: Simon Hawkins <[email protected]>

BUG: Fix behavior of replace_list with mixed types.

b035dd1

hasan-yaman mentioned this pull request Mar 21, 2021

BUG: Change in behavior of replace with integer series and float to_replace #40371

Closed

3 tasks

mzeitlin11 reviewed Mar 21, 2021

View reviewed changes

pandas/tests/series/methods/test_replace.py Outdated Show resolved Hide resolved

mzeitlin11 added Regression Functionality that used to work in a prior pandas version replace replace method labels Mar 21, 2021

dsaxton reviewed Mar 21, 2021

View reviewed changes

BUG: Revert changes in cast.py and refactor tests

fddddf6

jbrockmendel reviewed Mar 22, 2021

View reviewed changes

dsaxton reviewed Mar 23, 2021

View reviewed changes

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

simonjayhawkins added this to the 1.2.5 milestone Apr 12, 2021

github-actions bot added the Stale label May 14, 2021

simonjayhawkins added 2 commits May 24, 2021 14:23

Merge remote-tracking branch 'upstream/master' into replace-floats-wi…

5f52f01

…th-integers

add patch, release note and rename test variables

71dd789

simonjayhawkins mentioned this pull request May 24, 2021

BUG: Series[int].loc setitem with Series[int] results in Series[float] #41644

Merged

simonjayhawkins removed the Stale label May 24, 2021

parametrize

18d2b79

lbittarello reviewed May 24, 2021

View reviewed changes

simonjayhawkins reviewed Jun 1, 2021

View reviewed changes

simonjayhawkins added 2 commits June 1, 2021 12:19

Merge remote-tracking branch 'upstream/master' into replace-floats-wi…

4422cef

…th-integers

Merge remote-tracking branch 'upstream/master' into replace-floats-wi…

760f2d5

…th-integers

simonjayhawkins merged commit 9231b49 into pandas-dev:master Jun 1, 2021

This comment has been minimized.

Sign in to view

lumberbot-app bot added the Still Needs Manual Backport label Jun 1, 2021

simonjayhawkins pushed a commit to simonjayhawkins/pandas that referenced this pull request Jun 1, 2021

Backport PR pandas-dev#40555: BUG: Fix behavior of replace_list with …

a622a54

…mixed types.

simonjayhawkins mentioned this pull request Jun 1, 2021

Backport PR #40555: BUG: Fix behavior of replace_list with mixed types. #41761

Merged

simonjayhawkins removed the Still Needs Manual Backport label Jun 1, 2021

simonjayhawkins added a commit that referenced this pull request Jun 1, 2021

Backport PR #40555: BUG: Fix behavior of replace_list with mixed type…

7bc9992

…s. (#41761) Co-authored-by: hasan-yaman <[email protected]>

TLouf pushed a commit to TLouf/pandas that referenced this pull request Jun 1, 2021

BUG: Fix behavior of replace_list with mixed types. (pandas-dev#40555)

20325b6

Co-authored-by: Simon Hawkins <[email protected]>

JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Jul 3, 2021

BUG: Fix behavior of replace_list with mixed types. (pandas-dev#40555)

e308f89

Co-authored-by: Simon Hawkins <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix behavior of replace_list with mixed types. #40555

BUG: Fix behavior of replace_list with mixed types. #40555

hasan-yaman commented Mar 21, 2021

mzeitlin11 left a comment

mzeitlin11 commented Mar 21, 2021

dsaxton Mar 21, 2021

hasan-yaman Mar 21, 2021

hasan-yaman commented Mar 21, 2021

jbrockmendel Mar 22, 2021

simonjayhawkins commented Apr 12, 2021

jreback commented Apr 12, 2021

simonjayhawkins commented Apr 12, 2021

hasan-yaman commented Apr 12, 2021

simonjayhawkins commented Apr 12, 2021

jreback commented Apr 13, 2021

simonjayhawkins commented Apr 13, 2021

github-actions bot commented May 14, 2021

simonjayhawkins commented May 24, 2021

simonjayhawkins commented May 24, 2021 •

edited

Loading

simonjayhawkins commented May 24, 2021

simonjayhawkins commented May 24, 2021

lbittarello May 24, 2021

simonjayhawkins May 25, 2021 •

edited

Loading

simonjayhawkins commented May 26, 2021

simonjayhawkins Jun 1, 2021

simonjayhawkins commented Jun 1, 2021

jreback commented Jun 1, 2021

simonjayhawkins commented Jun 1, 2021

simonjayhawkins commented Jun 1, 2021

This comment has been minimized.

BUG: Fix behavior of replace_list with mixed types. #40555

BUG: Fix behavior of replace_list with mixed types. #40555

Conversation

hasan-yaman commented Mar 21, 2021

mzeitlin11 left a comment

Choose a reason for hiding this comment

mzeitlin11 commented Mar 21, 2021

dsaxton Mar 21, 2021

Choose a reason for hiding this comment

hasan-yaman Mar 21, 2021

Choose a reason for hiding this comment

hasan-yaman commented Mar 21, 2021

jbrockmendel Mar 22, 2021

Choose a reason for hiding this comment

simonjayhawkins commented Apr 12, 2021

jreback commented Apr 12, 2021

simonjayhawkins commented Apr 12, 2021

hasan-yaman commented Apr 12, 2021

simonjayhawkins commented Apr 12, 2021

jreback commented Apr 13, 2021

simonjayhawkins commented Apr 13, 2021

github-actions bot commented May 14, 2021

simonjayhawkins commented May 24, 2021

simonjayhawkins commented May 24, 2021 • edited Loading

simonjayhawkins commented May 24, 2021

simonjayhawkins commented May 24, 2021

lbittarello May 24, 2021

Choose a reason for hiding this comment

simonjayhawkins May 25, 2021 • edited Loading

Choose a reason for hiding this comment

simonjayhawkins commented May 26, 2021

simonjayhawkins Jun 1, 2021

Choose a reason for hiding this comment

simonjayhawkins commented Jun 1, 2021

jreback commented Jun 1, 2021

simonjayhawkins commented Jun 1, 2021

simonjayhawkins commented Jun 1, 2021

This comment has been minimized.

simonjayhawkins commented May 24, 2021 •

edited

Loading

simonjayhawkins May 25, 2021 •

edited

Loading