Fix series comparison operators when dealing with zero rank numpy arrays #13307
Conversation
|
Partially based on @sldion 's changes. Thanks |
| @@ -754,7 +754,8 @@ def wrapper(self, other, axis=None): | |||
| elif isinstance(other, pd.DataFrame): # pragma: no cover | |||
| return NotImplemented | |||
| elif isinstance(other, (np.ndarray, pd.Index)): | |||
| if len(self) != len(other): | |||
| lo = len(np.atleast_1d(other)) | |||
| if lo != 1 and len(self) != lo: | |||
There was a problem hiding this comment.
why are you adding this check for != 1? this seems to be hiding thing. Further why don't you do:
other = np.atleast_1d(other) and be done?
There was a problem hiding this comment.
len(other) == 1 shouldn't throw the exception, as other is broadcasted by _constructor ...
There was a problem hiding this comment.
much better to fix it than put this kind of conditional in
There was a problem hiding this comment.
how do you see this changed differently?
There was a problem hiding this comment.
I tried, but that fails the (new) testcase
Would this work?
try:
return self._constructor(na_op(self.values, np.asarray(other)),
index=self.index).__finalize__(self)
except:
raise ValueError('Lengths must match to compare')
There was a problem hiding this comment.
no that's not a good thing to do, we don't like non-specific try:excepts.
you can specifically deal with it by something like:
if lib.isscalar(lib.item_from_zerodim(other)):
There was a problem hiding this comment.
modified with this above
| if len(self) != len(other): | ||
| raise ValueError('Lengths must match to compare') | ||
| if not lib.isscalar(lib.item_from_zerodim(other)): | ||
| if len(self) != len(other): |
There was a problem hiding this comment.
still to much nested if statements
maybe just try to convert a 0-dim ndarray to a scaler instead
There was a problem hiding this comment.
there have to be two if statements, to determine scalar or not, and validate length being the same
There was a problem hiding this comment.
@jreback Would you like this to be changed?
There was a problem hiding this comment.
you can do this in an and. put a comment that need to care for zero-dim ndarrays
Current coverage is 84.22%@@ master #13307 diff @@
==========================================
Files 138 138
Lines 50721 50667 -54
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 42723 42673 -50
+ Misses 7998 7994 -4
Partials 0 0
|
| @@ -315,6 +315,6 @@ Bug Fixes | |||
| - Bug in ``groupby`` where ``apply`` returns different result depending on whether first result is ``None`` or not (:issue:`12824`) | |||
|
|
|||
|
|
|||
|
|
|||
| - Bug in series comparison operators when dealing with zero rank numpy arrays (:issue:`13006`) | |||
|
|
|||
There was a problem hiding this comment.
Use Series (surrounded by double-ticks) instead of "series" and change "numpy" to "NumPy"
|
@jreback Are you good with this PR? Thanks |
| @@ -264,6 +264,15 @@ def test_operators_timedelta64(self): | |||
| rs[2] += np.timedelta64(timedelta(minutes=5, seconds=1)) | |||
| self.assertEqual(rs[2], value) | |||
|
|
|||
| def test_operator_series_comparison_zerorank(self): | |||
| self.assert_series_equal((np.float64(0) > pd.Series([1, 2, 3])), (0.0 > | |||
There was a problem hiding this comment.
add the issue number as a comment
|
build is green |
| if len(self) != len(other): | ||
| # do not check length of zerorank array | ||
| if not lib.isscalar(lib.item_from_zerodim(other)) and \ | ||
| len(self) != len(other): | ||
| raise ValueError('Lengths must match to compare') |
There was a problem hiding this comment.
I've been nitpicked on this before, but @jreback is there a preference for backslash or parentheses in multi-line conditionals?
There was a problem hiding this comment.
either one are fine really. I guess in this case parens might be nicer. I'll fix on the merge
|
thanks @gliptak |
git diff upstream/master | flake8 --diff