Skip to content

REF: use unpack_zerodim_and_defer on EA methods #34042

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

Merged
merged 7 commits into from
Aug 7, 2020
+28 −33
Merged

REF: use unpack_zerodim_and_defer on EA methods #34042

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e56ddf9
REF: use unpack_zerodim_and_defer on EA methods
jbrockmendel May 7, 2020
86b26d2
Merge branch 'master' of https://github.com/pandas-dev/pandas into bo…
jbrockmendel Jun 2, 2020
1682b92
update test
jbrockmendel Jun 2, 2020
9ced7d4
Merge branch 'master' of https://github.com/pandas-dev/pandas into bo…
jbrockmendel Jun 7, 2020
0a4e835
Merge remote-tracking branch 'upstream/master' into boilerplate-4
jorisvandenbossche Jun 12, 2020
7796934
explicit defer in example implementation
jorisvandenbossche Jun 12, 2020
8b140f9
Merge remote-tracking branch 'upstream/master' into boilerplate-4
jorisvandenbossche Aug 6, 2020
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
4 changes: 2 additions & 2 deletions pandas/core/arrays/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
from pandas.core.dtypes.cast import maybe_cast_to_extension_array
from pandas.core.dtypes.common import is_array_like, is_list_like, pandas_dtype
from pandas.core.dtypes.dtypes import ExtensionDtype
from pandas.core.dtypes.generic import ABCIndexClass, ABCSeries
from pandas.core.dtypes.generic import ABCDataFrame, ABCIndexClass, ABCSeries
from pandas.core.dtypes.missing import isna

from pandas.core import ops
Expand Down Expand Up @@ -1273,7 +1273,7 @@ def convert_values(param):
ovalues = [param] * len(self)
return ovalues

if isinstance(other, (ABCSeries, ABCIndexClass)):
if isinstance(other, (ABCSeries, ABCIndexClass, ABCDataFrame)):
# rely on pandas to unbox and dispatch to us
return NotImplemented
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I wouldn't use the decorator here. This class is meant to be used by external EA authors (and in that way also serves somewhat as documentation), and it is not used internally (except for tests).
I find the "easy understandability" in this case more important than the small code duplication (so it is clear to EA authors this method is returning NotImplemented for pandas objects.

And you can add ABCDataFrame to the check to make it correct (and we should have a test to catch this ..)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think its fine to make this change. why have extra-boilerplate code

Copy link
Member

Choose a reason for hiding this comment

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

why have extra-boilerplate code

For the reason I gave in the above comment

Copy link
Member

Choose a reason for hiding this comment

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

Can we simply remove it here? (or actually give arguments for why you want to change it)
This is basically a toy/helper implementation that is not used internally.

(I am completely fine with the rest of the PR)

Copy link
Contributor

Choose a reason for hiding this comment

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

@jbrockmendel I am ok with this here, but I suppose @jorisvandenbossche is stronger here, I guess revert this part.; though this makes things a bit inconsistent across pandas which is odd


Expand Down
19 changes: 4 additions & 15 deletions pandas/core/arrays/boolean.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
pandas_dtype,
)
from pandas.core.dtypes.dtypes import register_extension_dtype
from pandas.core.dtypes.generic import ABCDataFrame, ABCIndexClass, ABCSeries
from pandas.core.dtypes.missing import isna

from pandas.core import ops
Expand Down Expand Up @@ -559,13 +558,10 @@ def all(self, skipna: bool = True, **kwargs):

@classmethod
def _create_logical_method(cls, op):
@ops.unpack_zerodim_and_defer(op.__name__)
def logical_method(self, other):
if isinstance(other, (ABCDataFrame, ABCSeries, ABCIndexClass)):
# Rely on pandas to unbox and dispatch to us.
return NotImplemented

assert op.__name__ in {"or_", "ror_", "and_", "rand_", "xor", "rxor"}
other = lib.item_from_zerodim(other)
other_is_booleanarray = isinstance(other, BooleanArray)
other_is_scalar = lib.is_scalar(other)
mask = None
Expand Down Expand Up @@ -605,16 +601,14 @@ def logical_method(self, other):

@classmethod
def _create_comparison_method(cls, op):
@ops.unpack_zerodim_and_defer(op.__name__)
def cmp_method(self, other):
from pandas.arrays import IntegerArray

if isinstance(
other, (ABCDataFrame, ABCSeries, ABCIndexClass, IntegerArray)
):
if isinstance(other, IntegerArray):
# Rely on pandas to unbox and dispatch to us.
return NotImplemented

other = lib.item_from_zerodim(other)
mask = None

if isinstance(other, BooleanArray):
Expand Down Expand Up @@ -693,13 +687,8 @@ def _maybe_mask_result(self, result, mask, other, op_name: str):
def _create_arithmetic_method(cls, op):
op_name = op.__name__

@ops.unpack_zerodim_and_defer(op_name)
def boolean_arithmetic_method(self, other):

if isinstance(other, (ABCDataFrame, ABCSeries, ABCIndexClass)):
# Rely on pandas to unbox and dispatch to us.
return NotImplemented

other = lib.item_from_zerodim(other)
mask = None

if isinstance(other, BooleanArray):
Expand Down
9 changes: 3 additions & 6 deletions pandas/core/arrays/numpy_.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,11 @@
from pandas.util._validators import validate_fillna_kwargs

from pandas.core.dtypes.dtypes import ExtensionDtype
from pandas.core.dtypes.generic import ABCIndexClass, ABCSeries
from pandas.core.dtypes.inference import is_array_like
from pandas.core.dtypes.missing import isna

from pandas import compat
from pandas.core import nanops
from pandas.core import nanops, ops
from pandas.core.algorithms import searchsorted
from pandas.core.array_algos import masked_reductions
from pandas.core.arrays._mixins import NDArrayBackedExtensionArray
Expand Down Expand Up @@ -436,11 +435,9 @@ def __invert__(self):

@classmethod
def _create_arithmetic_method(cls, op):
@ops.unpack_zerodim_and_defer(op.__name__)
def arithmetic_method(self, other):
if isinstance(other, (ABCIndexClass, ABCSeries)):
return NotImplemented

elif isinstance(other, cls):
if isinstance(other, cls):
other = other._ndarray

with np.errstate(all="ignore"):
Expand Down
8 changes: 3 additions & 5 deletions pandas/core/arrays/string_.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

from pandas.core.dtypes.base import ExtensionDtype, register_extension_dtype
from pandas.core.dtypes.common import pandas_dtype
from pandas.core.dtypes.generic import ABCDataFrame, ABCIndexClass, ABCSeries
from pandas.core.dtypes.inference import is_array_like

from pandas import compat
Expand Down Expand Up @@ -312,15 +311,14 @@ def memory_usage(self, deep=False):
@classmethod
def _create_arithmetic_method(cls, op):
# Note: this handles both arithmetic and comparison methods.

@ops.unpack_zerodim_and_defer(op.__name__)
def method(self, other):
from pandas.arrays import BooleanArray

assert op.__name__ in ops.ARITHMETIC_BINOPS | ops.COMPARISON_BINOPS

if isinstance(other, (ABCIndexClass, ABCSeries, ABCDataFrame)):
return NotImplemented

elif isinstance(other, cls):
if isinstance(other, cls):
other = other._ndarray

mask = isna(self) | isna(other)
Expand Down
15 changes: 11 additions & 4 deletions pandas/tests/extension/base/ops.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,13 @@ def test_error(self, data, all_arithmetic_operators):
with pytest.raises(AttributeError):
getattr(data, op_name)

def test_direct_arith_with_series_returns_not_implemented(self, data):
# EAs should return NotImplemented for ops with Series.
@pytest.mark.parametrize("box", [pd.Series, pd.DataFrame])
def test_direct_arith_with_ndframe_returns_not_implemented(self, data, box):
# EAs should return NotImplemented for ops with Series/DataFrame
# Pandas takes care of unboxing the series and calling the EA's op.
other = pd.Series(data)
if box is pd.DataFrame:
other = other.to_frame()
if hasattr(data, "__add__"):
result = data.__add__(other)
assert result is NotImplemented
Expand Down Expand Up @@ -156,10 +159,14 @@ def test_compare_array(self, data, all_compare_operators):
other = pd.Series([data[0]] * len(data))
self._compare_other(s, data, op_name, other)

def test_direct_arith_with_series_returns_not_implemented(self, data):
# EAs should return NotImplemented for ops with Series.
@pytest.mark.parametrize("box", [pd.Series, pd.DataFrame])
def test_direct_arith_with_ndframe_returns_not_implemented(self, data, box):
# EAs should return NotImplemented for ops with Series/DataFrame
# Pandas takes care of unboxing the series and calling the EA's op.
other = pd.Series(data)
if box is pd.DataFrame:
other = other.to_frame()

if hasattr(data, "__eq__"):
result = data.__eq__(other)
assert result is NotImplemented
Expand Down
6 changes: 5 additions & 1 deletion pandas/tests/extension/test_period.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,13 @@ def test_add_series_with_extension_array(self, data):
def test_error(self):
pass

def test_direct_arith_with_series_returns_not_implemented(self, data):
@pytest.mark.parametrize("box", [pd.Series, pd.DataFrame])
def test_direct_arith_with_ndframe_returns_not_implemented(self, data, box):
# Override to use __sub__ instead of __add__
other = pd.Series(data)
if box is pd.DataFrame:
other = other.to_frame()

result = data.__sub__(other)
assert result is NotImplemented

Expand Down