Skip to content

API: Index.__cmp__(Series) return NotImplemented #37160

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

Closed
wants to merge 1 commit into from
Closed

API: Index.__cmp__(Series) return NotImplemented #37160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 1 addition & 6 deletions pandas/core/indexes/extension.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
from pandas.util._decorators import cache_readonly, doc

from pandas.core.dtypes.common import is_dtype_equal, is_object_dtype
from pandas.core.dtypes.generic import ABCDataFrame, ABCSeries
from pandas.core.dtypes.generic import ABCDataFrame

from pandas.core.arrays import ExtensionArray
from pandas.core.indexers import deprecate_ndim_indexing
Expand Down Expand Up @@ -118,11 +118,6 @@ def _make_wrapped_comparison_op(opname: str):
"""

def wrapper(self, other):
if isinstance(other, ABCSeries):
# the arrays defer to Series for comparison ops but the indexes
# don't, so we have to unwrap here.
other = other._values

other = _maybe_unwrap_index(other)

op = getattr(self._data, opname)
Expand Down
15 changes: 5 additions & 10 deletions pandas/core/ops/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,20 +45,15 @@ def _unpack_zerodim_and_defer(method, name: str):
-------
method
"""
is_cmp = name.strip("__") in {"eq", "ne", "lt", "le", "gt", "ge"}

@wraps(method)
def new_method(self, other):

if is_cmp and isinstance(self, ABCIndexClass) and isinstance(other, ABCSeries):
# For comparison ops, Index does *not* defer to Series
pass
else:
for cls in [ABCDataFrame, ABCSeries, ABCIndexClass]:
if isinstance(self, cls):
break
if isinstance(other, cls):
return NotImplemented
for cls in [ABCDataFrame, ABCSeries, ABCIndexClass]:
if isinstance(self, cls):
break
if isinstance(other, cls):
return NotImplemented

other = item_from_zerodim(other)

Expand Down
9 changes: 6 additions & 3 deletions pandas/core/series.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4977,11 +4977,14 @@ def _cmp_method(self, other, op):

if isinstance(other, Series) and not self._indexed_same(other):
raise ValueError("Can only compare identically-labeled Series objects")
if isinstance(other, MultiIndex):
res_values = op(self._values, other)
else:
lvalues = extract_array(self, extract_numpy=True)

lvalues = extract_array(self, extract_numpy=True)
rvalues = extract_array(other, extract_numpy=True)
rvalues = extract_array(other, extract_numpy=True)

res_values = ops.comparison_op(lvalues, rvalues, op)
res_values = ops.comparison_op(lvalues, rvalues, op)

return self._construct_result(res_values, name=res_name)

Expand Down
8 changes: 5 additions & 3 deletions pandas/tests/arithmetic/test_datetime64.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,11 +744,13 @@ def test_dti_cmp_tdi_tzawareness(self, other):

result = dti == other
expected = np.array([False] * 10)
tm.assert_numpy_array_equal(result, expected)
if isinstance(other, Series):
Copy link
Contributor

Choose a reason for hiding this comment

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

so this is technically a small api change? e.g. a dti == Series will now give a boolean Series? do we do this elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

API change, yes. not just DTI, but any index == series will now return a Series[bool] instead of ndarray[bool]

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm then i am no sure I like this api change. We return bool ndarrays on purpose from comparisions to avoid having to align everything. I think this could potentially be a nasty user trap.

Is there a reason you want to do this (aside from reconciling / cleaning).

Copy link
Member Author

Choose a reason for hiding this comment

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

My motivation is pretty much "hunt down special cases"

IIRC @jorisvandenbossche mentioned a ways back that the existing behavior is from a time before series[bool_dtypes_series] was supported, but i could be off on that

expected = Series(expected, index=other.index)
tm.assert_equal(result, expected)

result = dti != other
expected = np.array([True] * 10)
tm.assert_numpy_array_equal(result, expected)
tm.assert_equal(result, ~expected)

msg = "Invalid comparison between"
with pytest.raises(TypeError, match=msg):
dti < other
Expand Down
4 changes: 2 additions & 2 deletions pandas/tests/indexes/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,8 +645,8 @@ def test_equals_op(self):
with pytest.raises(ValueError, match=msg):
index_a == series_b

tm.assert_numpy_array_equal(index_a == series_a, expected1)
tm.assert_numpy_array_equal(index_a == series_c, expected2)
tm.assert_series_equal(index_a == series_a, Series(expected1))
tm.assert_series_equal(index_a == series_c, Series(expected2))

# cases where length is 1 for one of them
with pytest.raises(ValueError, match="Lengths must match"):
Expand Down
4 changes: 2 additions & 2 deletions pandas/tests/indexes/multi/test_equivalence.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ def test_equals_op(idx):
with pytest.raises(ValueError, match="Lengths must match"):
index_a == series_b

tm.assert_numpy_array_equal(index_a == series_a, expected1)
tm.assert_numpy_array_equal(index_a == series_c, expected2)
tm.assert_series_equal(index_a == series_a, Series(expected1))
tm.assert_series_equal(index_a == series_c, Series(expected2))

# cases where length is 1 for one of them
with pytest.raises(ValueError, match="Lengths must match"):
Expand Down