BUG: inconsistent replace #36444
BUG: inconsistent replace #36444
Conversation
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
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
|
Thanks @QuentinN42 for the PR. needs tests see #35376 (comment) and a release note in doc/source/whatsnew/v1.1.3.rst |
Added the pandas-dev#35376 error as a test.
Test added. |
Added regression description in `doc/source/whatsnew/v1.1.3.rst`
moved 4 lines in once
| @@ -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]]),), | |||
There was a problem hiding this comment.
can you add the .replace(1, 5) case and maybe also .replace(1.0, 5.0) and .replace(1, 5.0)
Thx @simonjayhawkins for the typo Co-authored-by: Simon Hawkins <simonjayhawkins@gmail.com>
|
@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)
|
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
|
I've rolled back on typo from f64817b to pass the PEP8 test. |
Formatting is just non sense
|
How can we see wich tests have failed and what is the given output ? |
may be fixed by #36511 |
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 |
| @@ -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()) | |||
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
Also is this simply wrong:
[ins] In [3]: is_integer(1.0)
Out[3]: Falseespecially since
[ins] In [4]: 1.0.is_integer()
Out[4]: TrueThere was a problem hiding this comment.
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
There was a problem hiding this comment.
maybe could replace this entirely with element.is_integer() and call it a day
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
ok this is fine @QuentinN42 if you can add a comment here would be good
| @@ -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()) | |||
There was a problem hiding this comment.
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())
|
I have a feeling that there are reproducibility errors with black. On server : On local : The error come from this line ( that black want to split into : |
|
@QuentinN42 the version of black used on ci was changed yesterday. see #36493 updating black locally and merging upstream/master will probably fix. |
refactor
|
The error come from |
seen b4 #36382 (comment) restarted ci |
…istant-replace # Conflicts: # doc/source/whatsnew/v1.1.3.rst
|
@jreback green #36444 (review) |
pandas/core/internals/blocks.py
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
not what I meant.
say something like
we have not inferred an integer; check if we have an equal float
|
the current comment is not useful - needs to be updated |
|
I've changed into : |
|
@jreback ok to merge on green? |
|
thanks @QuentinN42 |
|
@meeseeksdev backport 1.1.x |
Co-authored-by: Number42 <32516498+QuentinN42@users.noreply.github.com>
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffdoc/source/whatsnew/v1.1.3.rstMust fix some replace in IntBlock if called with float integer (such as 1.0).
closes #35376