make ops.add_foo take just class #19828
Conversation
pandas/core/ops.py
Outdated
| ------- | ||
| arith_method : function | ||
| comp_method : function | ||
| bool_method : function |
There was a problem hiding this comment.
I think it would be better just have a dict here keyed on the cls and just returns the 5 methods. This seems a bit overkill
There was a problem hiding this comment.
Implementing this it ends up being pretty close to a wash. We have to key on cls._typ most of the time but also check for cls._subtyp for SparseSeries and SparseDataFrame. At this point I'm leaning towards "being a little bit verbose here is OK in this case". The main upside is its helpful to see which wrappers are used where without having to jump around the file.
There was a problem hiding this comment.
ok, can do in a single function then
jreback
added
Numeric Operations
Codecov Report
@@ Coverage Diff @@
## master #19828 +/- ##
=========================================
Coverage ? 91.68%
=========================================
Files ? 150
Lines ? 48943
Branches ? 0
=========================================
Hits ? 44875
Misses ? 4068
Partials ? 0
Continue to review full report at Codecov.
|
pandas/core/ops.py
Outdated
| ------- | ||
| arith_method : function | ||
| comp_method : function | ||
| bool_method : function |
There was a problem hiding this comment.
ok, can do in a single function then
pandas/core/ops.py
Outdated
| """ | ||
| arith_method, comp_method, bool_method = _get_method_wrappers(cls)[2:] |
There was a problem hiding this comment.
i'd rather you now do this an instead use _, _, ... for the return values its more obvious
|
|
||
| if is_integer_dtype(left) and is_integer_dtype(right): | ||
| # series coerces to float64 if result should have NaN/inf | ||
| if opname in ('floordiv', 'mod') and (right.values == 0).any(): |
There was a problem hiding this comment.
this should go thru the fill_zeros path.
There was a problem hiding this comment.
Is this obvious? I don't know the sparse code all that well, have been assuming that the do-it-upfront approach taken here was for a reason.
There was a problem hiding this comment.
There will need to be a pass soon to make Series division handle division by zero the way that Index division now does. That might be a reasonable time to take a close look at this.
There was a problem hiding this comment.
ok pls add it to the list.
|
|
||
| if is_integer_dtype(left) and is_integer_dtype(right): | ||
| # series coerces to float64 if result should have NaN/inf | ||
| if opname in ('floordiv', 'mod') and (right.values == 0).any(): |
There was a problem hiding this comment.
ok pls add it to the list.
As discussed
Also small cleanup for Sparse ops.