Fixing == __eq__ operator for MultiIndex ... closes #9785 #9872
Conversation
jreback
added
Error Reporting
pykler
changed the title
| @@ -2439,6 +2439,8 @@ def _get_dtype_type(arr_or_dtype): | |||
| return np.dtype(arr_or_dtype).type | |||
| elif isinstance(arr_or_dtype, CategoricalDtype): | |||
| return CategoricalDtypeType | |||
| elif arr_or_dtype is NotImplemented: | |||
| return type(NotImplemented) | |||
There was a problem hiding this comment.
This seems strange to me. Why would we ever want to use NotImplementedType?
There was a problem hiding this comment.
This function just returns the type of an object ... NotImplemented is an object. So not sure what the question is?
if the question is why is this feature important? Pandas has code that checks the type of return values and acts accordingly. Occasionally Numpy will return this type and this code breaks with an attribute error because NotImplementedType has no attribute dtype.
PS. Without this the tests below fail. This may not be clear from the small diff but the bug in this case is visible in _indexOp pandas/core/index.py:58
There was a problem hiding this comment.
OK, thanks for clarifying. My sense is that this method probably should raise if it gets NotImplemented, because this is not really a valid numpy dtype.
Instead, I would make the wrapper functions in pandas.core.index handle NotImplemented explicitly, e.g., add something like
if result is NotImplemented:
return result
Operations that wrap arithmetic should know how to handle NotImplemented -- that's a saner place to put it than here (where it could eat some strange errors)..
There was a problem hiding this comment.
I get what you are saying about numpy types, but it shouldn't eat up any errors, since this function just says that the type of the input is so and so ... right?
There was a problem hiding this comment.
Based purely on the name, function expects a numpy/pandas array or dtype object, and NotImplemented is certainly not one of those :). Returning arbitrary output instead of raising an error feels like an anti-pattern to me.
In contrast, for something like is_bool_dtype it does make sense to return False when given NotImplemented.
There was a problem hiding this comment.
Makes sense, I'll make this change in is_bool_dtype instead ... ccing @jreback for reference
| @@ -2510,6 +2510,8 @@ def is_floating_dtype(arr_or_dtype): | |||
|
|
|||
|
|
|||
| def is_bool_dtype(arr_or_dtype): | |||
| if arr_or_dtype is NotImplemented: | |||
There was a problem hiding this comment.
K, this seems more reasonable, though from my perspective I would probably still be happier explicitly handling NotImplemented inside the wrapper functions.
There was a problem hiding this comment.
So conceptually though, the code in the wrapper got a result, its trying to check if this result is a bool type ... the check is crashing on this NULL type of exception. So this is where its gets tricky conceptually, is NULL part of this function's domain or not?
There was a problem hiding this comment.
agreed... might be better to use try/except logic, e.g.,
try:
tipo = _get_dtype_type(arr_or_dtype)
except Exception:
return False
There was a problem hiding this comment.
sounds good, one last thing. I tested the following and it also crapped out.
_get_dtype_type(False)
so is this also supposed to crap out? or is it supposed to return False?
is_bool_dtype(False)
There was a problem hiding this comment.
Well, this is an internal function, so it's not extensively tested (and not worth worrying about testing too much). It expects the argument to already be a numpy dtype or numpy array.
There was a problem hiding this comment.
I caught ValueError and made sure that's raised ... does this look better?
|
merged via 07257a0 thanks! |
closes #9785