BUG: inconsistent replace #36444

QuentinN42 · 2020-09-18T06:50:27Z

tests added
tests passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
release note in doc/source/whatsnew/v1.1.3.rst

Must fix some replace in IntBlock if called with float integer (such as 1.0).

Added return is_integer(element) or is_float(element) to the IntBlock._can_hold_element method because an Block of ints can be replaced from int. Read pandas-dev#35376 for more info

@jbrockmendel

As @jbrockmendel said in pandas-dev#35376, you can replace an int by a float in an IntBlock only if the element is an integer.

move an import 2 lines above

simonjayhawkins · 2020-09-18T08:32:30Z

Thanks @QuentinN42 for the PR. needs tests see #35376 (comment) and a release note in doc/source/whatsnew/v1.1.3.rst

jreback

pls always add a test that reproduces the issue first to make sure it fails.

pandas/core/internals/blocks.py

Added the pandas-dev#35376 error as a test.

QuentinN42 · 2020-09-18T13:31:28Z

pls always add a test that reproduces the issue first to make sure it fails.

Test added.

Added regression description in `doc/source/whatsnew/v1.1.3.rst`

moved 4 lines in once

simonjayhawkins · 2020-09-18T15:09:24Z

pandas/tests/frame/methods/test_replace.py

@@ -974,6 +974,8 @@ def test_replace_for_new_dtypes(self, datetime_frame):
                    }
                ),
            ),
+            # GH 35376
+            (DataFrame([[1, 1.0], [2, 2.0]]), 1.0, 5, DataFrame([[5, 5.0], [2, 2.0]]),),


can you add the .replace(1, 5) case and maybe also .replace(1.0, 5.0) and .replace(1, 5.0)

doc/source/whatsnew/v1.1.3.rst

@simonjayhawkins

Thx @simonjayhawkins for the typo Co-authored-by: Simon Hawkins <[email protected]>

dsaxton · 2020-09-19T13:43:59Z

@QuentinN42 Can you merge master to fix merge conflict? Also I think this doesn't need to be a draft.

# Conflicts: # doc/source/whatsnew/v1.1.3.rst

added .replace(1, 5), .replace(1.0, 5.0) and .replace(1, 5.0)

pep8speaks · 2020-09-21T06:48:14Z

Hello @QuentinN42! 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-09-25 11:06:55 UTC

Moved all singles lines in 6 lines

QuentinN42 · 2020-09-21T07:00:41Z

I've rolled back on typo from f64817b to pass the PEP8 test.
But it may not pass the CI / Checks (PR)

Formatting is just non sense

QuentinN42 · 2020-09-21T08:35:14Z

How can we see wich tests have failed and what is the given output ?

simonjayhawkins · 2020-09-21T09:02:25Z

How can we see wich tests have failed and what is the given output ?

may be fixed by #36511

QuentinN42 · 2020-09-21T14:10:32Z

How can we see wich tests have failed and what is the given output ?

may be fixed by #36511

#36511 has been merged.

simonjayhawkins · 2020-09-21T14:17:43Z

#36511 has been merged.

i've restarted the tests since (azure normally picks up the changes in master) but it looks like there are still failures.

can you merge upstream/master into this branch

jreback · 2020-09-22T01:49:39Z

pandas/core/internals/blocks.py

@@ -2066,7 +2067,7 @@ def _can_hold_element(self, element: Any) -> bool:
                and not issubclass(tipo.type, (np.datetime64, np.timedelta64))
                and self.dtype.itemsize >= tipo.itemsize
            )
-        return is_integer(element)
+        return is_integer(element) or (is_float(element) and element.is_integer())


@jbrockmendel are you sure you think the patch should be here; this is hit by a lot of paths where this doesn't make any sense. we almost always want to take some sort of action based on return value here if it fails. e.g. why is this not more specific to replace?

Also is this simply wrong:

[ins] In [3]: is_integer(1.0) Out[3]: False

especially since

[ins] In [4]: 1.0.is_integer() Out[4]: True

ahh its doing an equality check under the hood, we are not

n [60]: 1.0.is_integer() Out[60]: True In [61]: 1.1.is_integer() Out[61]: False

maybe could replace this entirely with element.is_integer() and call it a day

though my concern is still that the purpose of this check is can we hold an integer, not are we a float and can be treated as an integer. though practically it may not make a difference.

So I don't realy understand ...
Need I replace return is_integer(element) into return element.is_integer().
If I do that, need I try except the AttributeError and return None ?

Or False ?

Longer-term we're going to have to look more carefully about can_hold_element and what we want it to mean. In the short run, for this check to be consistent with the tipo check above it should exclude floats

(so my previous suggestion may have been unhelpful, sorry)

ok this is fine @QuentinN42 if you can add a comment here would be good

jreback

pls ping on green

jreback · 2020-09-22T22:12:02Z

pandas/core/internals/blocks.py

@@ -2066,7 +2067,7 @@ def _can_hold_element(self, element: Any) -> bool:
                and not issubclass(tipo.type, (np.datetime64, np.timedelta64))
                and self.dtype.itemsize >= tipo.itemsize
            )
-        return is_integer(element)
+        return is_integer(element) or (is_float(element) and element.is_integer())


ok this is fine @QuentinN42 if you can add a comment here would be good

marking GH 36444 and GH 35376 close to (is_float(element) and element.is_integer())

QuentinN42 · 2020-09-23T07:33:47Z

I have a feeling that there are reproducibility errors with black.
On my personals repos, I've removed it from pre-commit for this errors.

On server :

black....................................................................Failed
- hook id: black
- files were modified by this hook

reformatted /home/runner/work/pandas/pandas/pandas/tests/frame/methods/test_replace.py
All done! ✨ 🍰 ✨
1 file reformatted, 1133 files left unchanged.

flake8...................................................................Passed
flake8-pyx...............................................................Passed
flake8-pxd...............................................................Passed
isort....................................................................Passed
pyupgrade................................................................Passed

On local :

black....................................................................Passed
flake8...................................................................Failed
- hook id: flake8
- exit code: 1

pandas/core/generic.py:1:1: F407 future feature annotations is not defined
pandas/core/algorithms.py:5:1: F407 future feature annotations is not defined
pandas/core/construction.py:7:1: F407 future feature annotations is not defined
pandas/core/frame.py:11:1: F407 future feature annotations is not defined

flake8-pyx...............................................................Passed
flake8-pxd...............................................................Passed
isort....................................................................Passed

The error come from this line (pandas/tests/frame/methods/test_replace.py):

(DataFrame([[1, 1.0], [2, 2.0]]), 1, 5, DataFrame([[5, 5.0], [2, 2.0]]),),

that black want to split into :

(
    DataFrame([[1, 1.0], [2, 2.0]]),
    1,
    5,
    DataFrame([[5, 5.0], [2, 2.0]]),
),

simonjayhawkins · 2020-09-23T08:30:39Z

@QuentinN42 the version of black used on ci was changed yesterday. see #36493

updating black locally and merging upstream/master will probably fix.

…istant-replace

refactor

QuentinN42 · 2020-09-23T10:28:45Z

The error come from pandas/tests/test_sorting.py::TestSafeSort::test_codes_out_of_bound.
Not me 😢 (I hope).

simonjayhawkins · 2020-09-23T10:31:44Z

The error come from pandas/tests/test_sorting.py::TestSafeSort::test_codes_out_of_bound.
Not me 😢 (I hope).

seen b4 #36382 (comment)

restarted ci

…istant-replace # Conflicts: # doc/source/whatsnew/v1.1.3.rst

simonjayhawkins · 2020-09-23T20:16:22Z

@jreback green #36444 (review)

simonjayhawkins

Thanks @QuentinN42 lgtm

jreback · 2020-09-23T20:35:26Z

pandas/core/internals/blocks.py

@@ -2066,7 +2067,8 @@ def _can_hold_element(self, element: Any) -> bool:
                and not issubclass(tipo.type, (np.datetime64, np.timedelta64))
                and self.dtype.itemsize >= tipo.itemsize
            )
-        return is_integer(element)
+        # Will have to be modified in the future see GH 36444 and GH 35376


not what I meant.
say something like

we have not inferred an integer; check if we have an equal float

jreback · 2020-09-24T00:15:33Z

the current comment is not useful - needs to be updated

QuentinN42 · 2020-09-24T09:35:11Z

I've changed into :

We have not inferred an integer from the dtype
check if we have a builtin int or a float equal to an int

…istant-replace

simonjayhawkins · 2020-09-25T11:08:04Z

@jreback ok to merge on green?

jreback · 2020-09-26T01:25:24Z

thanks @QuentinN42

simonjayhawkins · 2020-09-26T08:26:17Z

@meeseeksdev backport 1.1.x

Co-authored-by: Number42 <[email protected]>

QuentinN42 added 2 commits September 18, 2020 08:30

BUG: is_float added to IntBlock._can_hold_element

cf5fc6d

Added return is_integer(element) or is_float(element) to the IntBlock._can_hold_element method because an Block of ints can be replaced from int. Read pandas-dev#35376 for more info

BUG: is_float(e) and e.is_integer() added to IntBlock._can_hold_element

3222d60

As @jbrockmendel said in pandas-dev#35376, you can replace an int by a float in an IntBlock only if the element is an integer.

QuentinN42 marked this pull request as draft September 18, 2020 06:52

isort passed

6a047cf

move an import 2 lines above

simonjayhawkins added Bug Regression Functionality that used to work in a prior pandas version replace replace method labels Sep 18, 2020

simonjayhawkins added this to the 1.1.3 milestone Sep 18, 2020

jreback requested changes Sep 18, 2020

View reviewed changes

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

replace test added

ff63b40

Added the pandas-dev#35376 error as a test.

QuentinN42 added 2 commits September 18, 2020 15:55

release note

52ac6f5

Added regression description in `doc/source/whatsnew/v1.1.3.rst`

black on test_replace

f64817b

moved 4 lines in once

simonjayhawkins reviewed Sep 18, 2020

View reviewed changes

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

Update doc/source/whatsnew/v1.1.3.rst

23bd5ce

Thx @simonjayhawkins for the typo Co-authored-by: Simon Hawkins <[email protected]>

simonjayhawkins mentioned this pull request Sep 19, 2020

BUG: Fix replace for different dtype equal value #34878

Closed

5 tasks

QuentinN42 added 2 commits September 21, 2020 08:44

Merge branch 'master' into issue35376-inconsistant-replace

0ac9765

# Conflicts: # doc/source/whatsnew/v1.1.3.rst

more tests on test_replace

473f108

added .replace(1, 5), .replace(1.0, 5.0) and .replace(1, 5.0)

test_replace PEP8

1355628

Moved all singles lines in 6 lines

black on test_replace

8b86da4

Formatting is just non sense

QuentinN42 marked this pull request as ready for review September 21, 2020 13:48

jreback changed the title ~~Issue35376 inconsistant replace~~ BUG: inconsistent replace Sep 22, 2020

jreback reviewed Sep 22, 2020

View reviewed changes

jreback requested changes Sep 22, 2020

View reviewed changes

added comment in IntBlock._can_hold_element

b867681

marking GH 36444 and GH 35376 close to (is_float(element) and element.is_integer())

QuentinN42 added 2 commits September 23, 2020 11:20

Merge remote-tracking branch 'upstream/master' into issue35376-incons…

5759673

…istant-replace

black test_replace.py

417bbdb

refactor

Merge remote-tracking branch 'upstream/master' into issue35376-incons…

36e23d1

…istant-replace # Conflicts: # doc/source/whatsnew/v1.1.3.rst

simonjayhawkins approved these changes Sep 23, 2020

View reviewed changes

jreback requested changes Sep 23, 2020

View reviewed changes

changing comment to be more explicit

ada13b6

Merge remote-tracking branch 'upstream/master' into issue35376-incons…

13ad68e

…istant-replace

QuentinN42 requested review from jreback and simonjayhawkins September 24, 2020 09:50

Merge remote-tracking branch 'upstream/master' into issue35376-incons…

36a88fe

…istant-replace

jreback approved these changes Sep 26, 2020

View reviewed changes

jreback merged commit 04e07d0 into pandas-dev:master Sep 26, 2020

meeseeksmachine mentioned this pull request Sep 26, 2020

Backport PR #36444 on branch 1.1.x (BUG: inconsistent replace) #36658

Merged

meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Sep 26, 2020

Backport PR pandas-dev#36444: BUG: inconsistent replace

b395996

simonjayhawkins pushed a commit that referenced this pull request Sep 26, 2020

Backport PR #36444: BUG: inconsistent replace (#36658)

c5bd38d

Co-authored-by: Number42 <[email protected]>

kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020

BUG: inconsistent replace (pandas-dev#36444)

915cbac

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: inconsistent replace #36444

BUG: inconsistent replace #36444

QuentinN42 commented Sep 18, 2020 •

edited

Loading

simonjayhawkins commented Sep 18, 2020

jreback left a comment

QuentinN42 commented Sep 18, 2020

simonjayhawkins Sep 18, 2020

dsaxton commented Sep 19, 2020

pep8speaks commented Sep 21, 2020 •

edited

Loading

QuentinN42 commented Sep 21, 2020

QuentinN42 commented Sep 21, 2020

simonjayhawkins commented Sep 21, 2020

QuentinN42 commented Sep 21, 2020

simonjayhawkins commented Sep 21, 2020

jreback Sep 22, 2020

dsaxton Sep 22, 2020

jreback Sep 22, 2020

jreback Sep 22, 2020

jreback Sep 22, 2020

QuentinN42 Sep 22, 2020

QuentinN42 Sep 22, 2020

jbrockmendel Sep 22, 2020

jreback Sep 22, 2020

jreback left a comment

jreback Sep 22, 2020

QuentinN42 commented Sep 23, 2020

simonjayhawkins commented Sep 23, 2020

QuentinN42 commented Sep 23, 2020

simonjayhawkins commented Sep 23, 2020

simonjayhawkins commented Sep 23, 2020

simonjayhawkins left a comment

jreback Sep 23, 2020

jreback commented Sep 24, 2020

QuentinN42 commented Sep 24, 2020

simonjayhawkins commented Sep 25, 2020 •

edited

Loading

jreback commented Sep 26, 2020

simonjayhawkins commented Sep 26, 2020

BUG: inconsistent replace #36444

BUG: inconsistent replace #36444

Conversation

QuentinN42 commented Sep 18, 2020 • edited Loading

simonjayhawkins commented Sep 18, 2020

jreback left a comment

Choose a reason for hiding this comment

QuentinN42 commented Sep 18, 2020

Choose a reason for hiding this comment

dsaxton commented Sep 19, 2020

pep8speaks commented Sep 21, 2020 • edited Loading

Comment last updated at 2020-09-25 11:06:55 UTC

QuentinN42 commented Sep 21, 2020

QuentinN42 commented Sep 21, 2020

simonjayhawkins commented Sep 21, 2020

QuentinN42 commented Sep 21, 2020

simonjayhawkins commented Sep 21, 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

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuentinN42 commented Sep 23, 2020

simonjayhawkins commented Sep 23, 2020

QuentinN42 commented Sep 23, 2020

simonjayhawkins commented Sep 23, 2020

simonjayhawkins commented Sep 23, 2020

simonjayhawkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Sep 24, 2020

QuentinN42 commented Sep 24, 2020

simonjayhawkins commented Sep 25, 2020 • edited Loading

jreback commented Sep 26, 2020

simonjayhawkins commented Sep 26, 2020

QuentinN42 commented Sep 18, 2020 •

edited

Loading

pep8speaks commented Sep 21, 2020 •

edited

Loading

simonjayhawkins commented Sep 25, 2020 •

edited

Loading