REF/CLN: ops boilerplate #23853 #24846
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24846 +/- ##
==========================================
- Coverage 92.39% 92.38% -0.01%
==========================================
Files 166 166
Lines 52378 52400 +22
==========================================
+ Hits 48393 48412 +19
- Misses 3985 3988 +3
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #24846 +/- ##
==========================================
- Coverage 92.38% 92.38% -0.01%
==========================================
Files 166 166
Lines 52398 52417 +19
==========================================
+ Hits 48406 48423 +17
- Misses 3992 3994 +2
Continue to review full report at Codecov.
|
pandas/core/arrays/base.py
Outdated
| @@ -1118,3 +1119,58 @@ def _create_arithmetic_method(cls, op): | |||
| @classmethod | |||
| def _create_comparison_method(cls, op): | |||
| return cls._create_method(op, coerce_to_dtype=False) | |||
|
|
|||
|
|
|||
| class CompWrapper(object): | |||
There was a problem hiding this comment.
this should go in core.ops
pandas/core/arrays/base.py
Outdated
| @wraps(comp) | ||
| def wrapper(comp_self, comp_other): | ||
| if isinstance(comp_other, (ABCDataFrame, | ||
| ABCSeries, ABCIndexClass)): |
There was a problem hiding this comment.
this is only accurate if comp_self is an EA. Also consider Index and Series
|
To close #23853 this would need to handle arithmetic ops in addition to comparison ops |
pandas/core/arrays/integer.py
Outdated
| @@ -496,22 +496,17 @@ def _values_for_argsort(self): | |||
|
|
|||
| @classmethod | |||
| def _create_comparison_method(cls, op): | |||
| @CompWrapper(validate_len=True, inst_from_senior_cls=True) | |||
There was a problem hiding this comment.
this changes the behavior to defer to DataFrame as well as Series and Index. I agree that it should do this eventually, but IIRC there are tests that fail if we start doing it now.
|
Any reason not to wrap Categorical comparison ops? |
gfyoung
added
Refactor
| @@ -136,6 +137,62 @@ def maybe_upcast_for_op(obj): | |||
| return obj | |||
|
|
|||
|
|
|||
| class CompWrapper(object): | |||
There was a problem hiding this comment.
Haven't looked at implementation in detail but can you add a docstring for this class to describe utility and parameters? Also what is the reasoning for going this route instead of say a meta class?
| @@ -1143,7 +1140,7 @@ def _time_shift(self, periods, freq=None): | |||
|
|
|||
| Note this is different from ExtensionArray.shift, which | |||
| shifts the *position* of each element, padding the end with | |||
| missing values. | |||
| missing values.x | |||
| 'zerodim', 'inst_from_senior_cls'] | ||
|
|
||
| def __init__(self, | ||
| list_to_array=None, |
There was a problem hiding this comment.
can you make these arg names even more explict, e.g.
zerodim -> unwrap_zero_dim_arrays
validate_len -> validate_len_of_input_arrays
inst_from_senior_cls -> not really sure what this does, can you make more explicit
list_to_array -> same
| return comp(comp_self, comp_other) | ||
| return wrapper | ||
|
|
||
| def _inst_from_senior_cls(self, comp): |
There was a problem hiding this comment.
add a doc-string to each of these
|
@jbrockmendel if you'd have a look. |
|
@jreback I'm -1 on this as implemented. The point of #23853 is to make all the methods behave the same; this does not do that. And while it technically removes boilerplate, it does so with a net increase in LOC. Even if merged, this does not close #23853, since there are many methods this isn't applied to. A better approach would be to write a single decorator with the desired behavior (like #24282) and apply it in subset of methods where it wouldn't change behavior, then gradually expand that subset by fixing the methods with idiosyncratic behavior. |
|
@makbigc if you want to respond to @jbrockmendel comments, alternatively can close. |
|
closing, @makbigc a followup according to @jbrockmendel comments would be ok |
git diff upstream/master -u -- "*.py" | flake8 --diffImplement a wrapper class other than a single decorator. It is tidier to add more conditions for ops later.