WIP: Use dispatch_to_series for combine_const #22751
Conversation
|
Hello @jbrockmendel! Thanks for updating the PR.
Comment last updated on October 03, 2018 at 21:58 Hours UTC |
| return ops.dispatch_to_series(self, right, func) | ||
|
|
||
| elif (np.ndim(other) == 1 and | ||
| isinstance(other, (tuple, np.ndarray)) and |
There was a problem hiding this comment.
There is a test for that goes through this path with a tuple, but none with a list. IIRC, no tests fail if list is included here.
Similarly, in the case above, only np.ndarray cases are tested. Should tuple and/or list be allowed?
| return ops.dispatch_to_series(self, other, func) | ||
|
|
||
| elif (np.ndim(other) == 2 and isinstance(other, np.ndarray) and | ||
| other.shape[0] == 1 and other.shape[1] == len(self.columns)): |
There was a problem hiding this comment.
Should the transposed case be supported?
|
Likely the easiest way to address the inconsistencies (and ugliness that goes with it) would be to call |
Codecov Report
@@ Coverage Diff @@
## master #22751 +/- ##
==========================================
- Coverage 92.18% 92.14% -0.05%
==========================================
Files 169 169
Lines 50823 50805 -18
==========================================
- Hits 46853 46816 -37
- Misses 3970 3989 +19
Continue to review full report at Codecov.
|
gfyoung
added
Internals
|
I am personally a bit hesitant to do much refactoring on something rather fragile as broadcasting behaviour without some better testing/documenting of the actual/desired broadcasting behaviour (but it might be a good opportunity to actually do that!). |
I definitely agree. In case it is not clear, this PR is not intended to be merged in its present form. The idea behind the ugly, verbose implementation here is to show precisely what behavior is currently tested. The discussion I'm hoping for is to clarify what the "spec" is supposed to be so that can be documented/tested and then implemented cleanly. |
|
OK, good to hear.
I think it would be a good idea to try to translate the 'ugly code' you wrote now to a written explanation of the broadcasting rules. |
This and #22019 get rid of the only two usages of
BlockManager.evalandBlock.eval. Getting rid of those will be a good day.This is pretty ugly at the moment. There are some design issues (namely, what broadcasting rules are intended to be supported) that need to be cleared up before this can be made pretty.