Skip to content

BUG: inconsistent replace #36444

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
Sep 26, 2020
Merged

BUG: inconsistent replace #36444

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
cf5fc6d
BUG: is_float added to IntBlock._can_hold_element
QuentinN42 Sep 18, 2020
3222d60
BUG: is_float(e) and e.is_integer() added to IntBlock._can_hold_element
QuentinN42 Sep 18, 2020
6a047cf
isort passed
QuentinN42 Sep 18, 2020
ff63b40
replace test added
QuentinN42 Sep 18, 2020
52ac6f5
release note
QuentinN42 Sep 18, 2020
f64817b
black on test_replace
QuentinN42 Sep 18, 2020
23bd5ce
Update doc/source/whatsnew/v1.1.3.rst
QuentinN42 Sep 18, 2020
0ac9765
Merge branch 'master' into issue35376-inconsistant-replace
QuentinN42 Sep 21, 2020
473f108
more tests on test_replace
QuentinN42 Sep 21, 2020
1355628
test_replace PEP8
QuentinN42 Sep 21, 2020
8b86da4
black on test_replace
QuentinN42 Sep 21, 2020
88c8f7e
Merge remote-tracking branch 'upstream/master' into issue35376-incons…
QuentinN42 Sep 21, 2020
b867681
added comment in IntBlock._can_hold_element
QuentinN42 Sep 23, 2020
5759673
Merge remote-tracking branch 'upstream/master' into issue35376-incons…
QuentinN42 Sep 23, 2020
417bbdb
black test_replace.py
QuentinN42 Sep 23, 2020
36e23d1
Merge remote-tracking branch 'upstream/master' into issue35376-incons…
QuentinN42 Sep 23, 2020
ada13b6
changing comment to be more explicit
QuentinN42 Sep 24, 2020
13ad68e
Merge remote-tracking branch 'upstream/master' into issue35376-incons…
QuentinN42 Sep 24, 2020
36a88fe
Merge remote-tracking branch 'upstream/master' into issue35376-incons…
simonjayhawkins Sep 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/source/whatsnew/v1.1.3.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ Fixed regressions
- Fixed regression in :meth:`Series.__getitem__` incorrectly raising when the input was a tuple (:issue:`35534`)
- Fixed regression in :meth:`Series.__getitem__` incorrectly raising when the input was a frozenset (:issue:`35747`)
- Fixed regression in :meth:`read_excel` with ``engine="odf"`` caused ``UnboundLocalError`` in some cases where cells had nested child nodes (:issue:`36122`, :issue:`35802`)
- Fixed regression in :meth:`DataFrame.replace` inconsistent replace when using a float in the replace method (:issue:`35376`)
- Fixed regression in :class:`DataFrame` and :class:`Series` comparisons between numeric arrays and strings (:issue:`35700`, :issue:`36377`)
- Fixed regression when setting empty :class:`DataFrame` column to a :class:`Series` in preserving name of index in frame (:issue:`36527`)
- Fixed regression in :class:`Period` incorrect value for ordinal over the maximum timestamp (:issue:`36430`)
Expand Down
4 changes: 3 additions & 1 deletion pandas/core/internals/blocks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
is_datetime64tz_dtype,
is_dtype_equal,
is_extension_array_dtype,
is_float,
is_float_dtype,
is_integer,
is_integer_dtype,
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not what I meant.
say something like

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

return is_integer(element) or (is_float(element) and element.is_integer())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or False ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



class DatetimeLikeBlockMixin:
Expand Down
25 changes: 25 additions & 0 deletions pandas/tests/frame/methods/test_replace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -974,6 +974,31 @@ 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]]),
),
(
DataFrame([[1, 1.0], [2, 2.0]]),
1,
5,
DataFrame([[5, 5.0], [2, 2.0]]),
),
(
DataFrame([[1, 1.0], [2, 2.0]]),
1.0,
5.0,
DataFrame([[5, 5.0], [2, 2.0]]),
),
(
DataFrame([[1, 1.0], [2, 2.0]]),
1,
5.0,
DataFrame([[5, 5.0], [2, 2.0]]),
),
],
)
def test_replace_dtypes(self, frame, to_replace, value, expected):
Expand Down