BUG: df.replace over pd.Period columns (#34871) #36867

samc1213 · 2020-10-04T19:01:03Z

closes BUG: Inconsistent behavior for df.replace over pd.Period columns #34871
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

This commit ensures that PeriodArrays return False for _can_hold_element for any element that is not a pd.Period. This prevents upstream code from casting the dtype to object. Also un-xfail test written in #34904

samc1213 · 2020-10-04T19:02:00Z

This is my first PR, so I would really appreciate guidance on how to improve the commit, and/or feedback on any process I am not following properly. Thanks in advance

pandas/core/internals/blocks.py

jbrockmendel · 2020-10-04T22:36:29Z

This is my first PR, so I would really appreciate guidance on how to improve the commit, and/or feedback on any process I am not following properly. Thanks in advance

You're doing well so far. A couple of comments, then the next thing to do will be to add a note in doc/source/whatsnew/1.2.0.rst giving a short description of the fixed bug

pandas/tests/frame/methods/test_replace.py

samc1213 · 2020-10-05T00:47:47Z

This is my first PR, so I would really appreciate guidance on how to improve the commit, and/or feedback on any process I am not following properly. Thanks in advance

You're doing well so far. A couple of comments, then the next thing to do will be to add a note in doc/source/whatsnew/1.2.0.rst giving a short description of the fixed bug

Thanks for your feedback, addressing your comments now. Also, what is the cause of the build failures? They seem to be happening to everyone, is someone working on fixing them? I was trying to find documentation of the build issue, if it is in fact a widespread thing.

pep8speaks · 2020-10-05T01:09:18Z

Hello @samc1213! 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 2020-11-16 03:22:57 UTC

jbrockmendel · 2020-10-05T17:43:49Z

Also, what is the cause of the build failures? They seem to be happening to everyone, is someone working on fixing them?

Yes, don't worry about it (already fixed in fact)

samc1213 · 2020-10-05T19:27:40Z

Pardon my ignorance, is there anything else I need to address/do to get this PR pulled? Thanks for all your help @jbrockmendel

jreback

can u run some asvs (indexing / reshaping ones)

this could potentially cause some perf issues

jreback

also there was a reference in the OP to #35268, if this fixes as well would be great to add the test (if not ok too)

jreback · 2020-10-06T22:49:27Z

doc/source/whatsnew/v1.2.0.rst

+Period
+^^^^^^
+
+- Bug in :meth:`DataFrame.replace` and :meth:`Series.replace` where :class:`Period` dtypes would be converted to object dytpes (:issue:34871)


typo on dtypes

dytpes

typo is still there. BTW in general let the person who made the original comment hit the "conversation resolved" button

Gotcha thanks for the heads up, haven't used github at all really. Fixed

pandas/tests/frame/methods/test_replace.py

pandas/tests/series/methods/test_replace.py

samc1213 · 2020-10-08T20:19:47Z

can u run some asvs (indexing / reshaping ones)

this could potentially cause some perf issues

Is there a server to run these on? Having issues running these locally in my docker env

jbrockmendel · 2020-10-09T00:00:13Z

Is there a server to run these on?

No.

Having issues running these locally in my docker env

are you familiar with using ipython's %timeit?

jreback · 2020-10-14T12:56:58Z

can you merge master

samc1213 · 2020-10-14T12:59:24Z

can you merge master

Will do, sorry I've taken so long on this, haven't had much time to try and get the ASVs working

samc1213 · 2020-10-21T23:02:27Z

also there was a reference in the OP to #35268, if this fixes as well would be great to add the test (if not ok too)

It does not fix #35268 - however, I think it's a very similar code change to fix - I can take care of that once this is approved

jreback · 2020-10-31T19:07:49Z

pandas/core/internals/blocks.py

+    def _can_hold_element(self, element: Any) -> bool:
+        if is_valid_nat_for_dtype(element, self.dtype):
+            return True
+        if element is NaT:


@jbrockmendel ok here?

@samc1213 are all of these branches actually hit? in particular why L2008 and L2010

@jreback Added these due to unit tests. Specifically, L2010 is required for pandas/tests/extension/test_interval.py::TestSetitem::test_setitem_empty_indxer to pass. L2008 may not be required, I just removed it and will see if the tests pass...

jreback · 2020-11-02T01:02:15Z

lgtm. @jbrockmendel if you'd look and merge if ok.

jreback · 2020-11-04T02:51:40Z

cc @jbrockmendel ok here

jbrockmendel · 2020-11-04T19:13:05Z

will take a look this afternoon

jbrockmendel · 2020-11-04T19:13:35Z

doc/source/whatsnew/v1.2.0.rst

@@ -434,6 +434,8 @@ Strings

 Interval
 ^^^^^^^^
+
+- Bug in :meth:`DataFrame.replace` and :meth:`Series.replace` where :class:`Interval` dtypes would be converted to object dtypes (:issue:34871)


backticks around the issue number

Thanks, fixed

pandas/core/internals/blocks.py

jbrockmendel · 2020-11-04T19:45:00Z

pandas/tests/series/methods/test_replace.py

+        Regression test for corrolary to GH#34871: if series.replace(1.0, 0.0)
+        is called on a Period/Interval Series, the old, faulty behavior
+        is to raise TypeError.
+        """


this can be a comment, not a docstring, just needs to see # GH#34871, ditto above

actually you can share this test using the frame_or_series fixture

Changed to a comment, but I grepped for frame_or_series and not finding much in the codebase. I did find index_or_series. Can you please elaborate more on how to use frame_or_series? There do seem to be a fair amount of duplication between these two files (series/test_replace and frame/test_replace). test_replace_with_compiled_regex is basically copy-pasted between the two, for example

Can you please elaborate more on how to use frame_or_series?

It's a pytest fixture:

pandas/pandas/conftest.py

Line 296 in aa390ec

def frame_or_series(request):

Thanks @arw2019 - I needed to merge master in locally to find this

jbrockmendel · 2020-11-11T22:47:57Z

pandas/tests/generic/methods/test_replace.py

+        regex = re.compile("^a$")
+        result = s.replace({regex: "z"}, regex=True)
+        expected = pd.Series(["z", "b", "c"])
+        tm.assert_series_equal(result, expected)


tests only belong in tests.generic.methods if all tests for that method are fully parametrized over DataFrame/Series, which these are not.

(i think this is confusing, so will likely just get rid of this directory at some point

I see, makes sense then. I'll just test frame and series in the same file then

jbrockmendel · 2020-11-11T22:49:13Z

pandas/tests/frame/methods/test_replace.py

@@ -1610,14 +1597,6 @@ def test_replace_dict_category_type(self, input_category_df, expected_category_d

        tm.assert_frame_equal(result, expected)

-    def test_replace_with_compiled_regex(self):


this is unrelated to the current PR, right? if you want to make a separate PR to parametrize this test, go for it. better to just use frame_or_series in this file and remove the version in the series tests

Sorry, yeah its unrelated. Undone, and just used frame_or_series here, just trying to organize it properly (evidently was misguided :D )

evidently was misguided

not at all, organizing/parametrizing/breaking-large-ancient-tests-into-specific-tests is something we very much want to encourage. just in dedicated PRs

I see, definitely makes sense - thanks

jbrockmendel · 2020-11-12T01:24:47Z

pandas/tests/frame/methods/test_replace.py

+    @pytest.mark.parametrize("value", [pd.Period("2020-01"), pd.Interval(0, 5)])
+    def test_replace_ea_ignore_float(self, frame_or_series, value):
+        # GH#34871
+        df = frame_or_series([value] * 3)
        result = df.replace(1.0, 0.0)


since this is now frame_or_series, can you call it obj instead of df

Got it, done

jbrockmendel · 2020-11-12T01:25:01Z

One last nitpick, otherwise LGTM, cc @jreback

jreback · 2020-11-17T01:17:34Z

thanks @samc1213

samc1213 · 2020-11-17T01:31:28Z

@jreback @jbrockmendel Thanks so much for your patience and feedback, this was a cool experience for me. Hopefully I'll be back! Appreciate all the time you put into this great project

BUG: df.replace over pd.Period columns (pandas-dev#34871)

a26823f

jbrockmendel reviewed Oct 4, 2020

View reviewed changes

pandas/core/internals/blocks.py Show resolved Hide resolved

jbrockmendel reviewed Oct 4, 2020

View reviewed changes

pandas/core/internals/blocks.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Oct 4, 2020

View reviewed changes

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

Remove PeriodExtensionBlock, update docs/tests

7b40ece

samc1213 added 2 commits October 5, 2020 01:11

Fix PEP violations

fa20da9

Fix docs: method to meth

c378787

jreback requested changes Oct 6, 2020

View reviewed changes

jreback added Period Period data type Bug replace replace method labels Oct 6, 2020

jreback requested changes Oct 6, 2020

View reviewed changes

samc1213 added 4 commits October 15, 2020 01:14

Merge branch 'master' of https://github.com/pandas-dev/pandas into 34871

6f38780

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

96f35f6

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

8be0a16

Address test and release notes comments

0b2de8f

samc1213 closed this Oct 21, 2020

samc1213 reopened this Oct 21, 2020

samc1213 mentioned this pull request Oct 21, 2020

BUG: pd.replace changes category dtype to object when using a dict to replace values #35268

Closed

3 tasks

jreback added this to the 1.2 milestone Oct 31, 2020

Merge branch 'master' into 34871

33a05af

jreback requested changes Oct 31, 2020

View reviewed changes

Remove excess NaT check

1d798bc

jreback approved these changes Nov 2, 2020

View reviewed changes

jbrockmendel reviewed Nov 4, 2020

View reviewed changes

pandas/core/internals/blocks.py Show resolved Hide resolved

jbrockmendel reviewed Nov 4, 2020

View reviewed changes

samc1213 added 6 commits November 10, 2020 03:07

Address review comments

5921fb7

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

8d16db5

Combine test_replace Series and DataFrame tests

91ad6dd

Add missing file

90ab53e

Remove unused import

f7bd1e4

Newline at end of file

bd1fea8

jbrockmendel reviewed Nov 11, 2020

View reviewed changes

samc1213 added 2 commits November 12, 2020 01:17

Remove generic unit test

0fa2a25

Add back series regex test

bcf61e6

jbrockmendel reviewed Nov 12, 2020

View reviewed changes

samc1213 added 3 commits November 12, 2020 01:26

Nitpick df to obj

42e8c74

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

d4c7acf

Fix conflict

5cc38c8

jreback merged commit d45903e into pandas-dev:master Nov 17, 2020

samc1213 deleted the 34871 branch November 17, 2020 01:31

		@@ -1610,14 +1597,6 @@ def test_replace_dict_category_type(self, input_category_df, expected_category_d

		tm.assert_frame_equal(result, expected)

		def test_replace_with_compiled_regex(self):

BUG: df.replace over pd.Period columns (#34871) #36867

BUG: df.replace over pd.Period columns (#34871) #36867

Conversation

samc1213 commented Oct 4, 2020 • edited Loading

samc1213 commented Oct 4, 2020

jbrockmendel commented Oct 4, 2020

samc1213 commented Oct 5, 2020

pep8speaks commented Oct 5, 2020 • edited Loading

Comment last updated at 2020-11-16 03:22:57 UTC

jbrockmendel commented Oct 5, 2020

samc1213 commented Oct 5, 2020

jreback left a comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samc1213 commented Oct 8, 2020

jbrockmendel commented Oct 9, 2020

jreback commented Oct 14, 2020

samc1213 commented Oct 14, 2020

samc1213 commented Oct 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Nov 2, 2020

jreback commented Nov 4, 2020

jbrockmendel commented Nov 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Nov 12, 2020

jreback commented Nov 17, 2020

samc1213 commented Nov 17, 2020

samc1213 commented Oct 4, 2020 •

edited

Loading

pep8speaks commented Oct 5, 2020 •

edited

Loading