CLN: get parts of Block.replace out of try/except #27408
Conversation
pandas/core/internals/blocks.py
Outdated
| @@ -733,6 +733,13 @@ def _try_coerce_args(self, other): | |||
| type(self).__name__.lower().replace("Block", ""), | |||
| ) | |||
| ) | |||
| if lib.is_scalar(other) and isna(other) and self.is_integer: | |||
There was a problem hiding this comment.
this seems way too specific. where is the above expression (np.any(...)) and not self._can_hold_element(other)
not true?
There was a problem hiding this comment.
Because its looking at NAs in the opposite way. The check above is for notna, whereas here we want to specifically catch np.nan
There was a problem hiding this comment.
I have no problem with the is_scalar and isna, its the is_integer which is way too specific here
There was a problem hiding this comment.
Totally agree that it shouldn't be necessary. In an earlier draft I had not self._can_hold_na instead of self.is_integer, but then BoolBlock started breaking. I had planned to revisit this in another step, but would also be OK with trying to get this to the better not self._can_hold_na in this step too.
There was a problem hiding this comment.
ok, yes I think if you used _can_hold_na here would be ok with this change.
There was a problem hiding this comment.
OK I was able to make this prettier, but at the cost of making a check in Block.where uglier. I think this is going to continue to be whack-a-mole until we can finish getting rid of the "try"s
| # GH 22083, TypeError or ValueError occurred within error handling | ||
| # causes infinite loop. Cast and retry only if not objectblock. | ||
| if is_object_dtype(self): | ||
| raise | ||
| raise AssertionError |
There was a problem hiding this comment.
this makes this block different than all others, why?
There was a problem hiding this comment.
in the status quo, raise re-raises the exception that is caught on 789. But now we don't have that to re-raise, so we need to raise a new exception. The raise on 793 is not reached (and should not be reachable)
There was a problem hiding this comment.
why don't we instead define an internal exception, say DtypeInvalidException (or better name) which inherits this then for flow control.
pandas/core/internals/blocks.py
Outdated
| @@ -1401,7 +1410,10 @@ def where(self, other, cond, align=True, errors="raise", try_cast=False, axis=0) | |||
|
|
|||
| # our where function | |||
| def func(cond, values, other): | |||
| other = self._try_coerce_args(other) | |||
|
|
|||
| if not (self.is_integer and lib.is_scalar(other) and np.isnan(other)): | |||
There was a problem hiding this comment.
seems way too specific here (the is_integer check)
There was a problem hiding this comment.
I agree, at first had not self._can_hold_na, but it turns out that breaks for BoolBlock
There was a problem hiding this comment.
same issue with the is_integer
pandas/core/internals/blocks.py
Outdated
| @@ -733,6 +733,13 @@ def _try_coerce_args(self, other): | |||
| type(self).__name__.lower().replace("Block", ""), | |||
| ) | |||
| ) | |||
| if lib.is_scalar(other) and isna(other) and self.is_integer: | |||
There was a problem hiding this comment.
I have no problem with the is_scalar and isna, its the is_integer which is way too specific here
| # GH 22083, TypeError or ValueError occurred within error handling | ||
| # causes infinite loop. Cast and retry only if not objectblock. | ||
| if is_object_dtype(self): | ||
| raise | ||
| raise AssertionError |
There was a problem hiding this comment.
why don't we instead define an internal exception, say DtypeInvalidException (or better name) which inherits this then for flow control.
pandas/core/internals/blocks.py
Outdated
| @@ -1401,7 +1410,10 @@ def where(self, other, cond, align=True, errors="raise", try_cast=False, axis=0) | |||
|
|
|||
| # our where function | |||
| def func(cond, values, other): | |||
| other = self._try_coerce_args(other) | |||
|
|
|||
| if not (self.is_integer and lib.is_scalar(other) and np.isnan(other)): | |||
There was a problem hiding this comment.
same issue with the is_integer
|
ok, can you merge master and ping on green. |
|
ping |
No description provided.