-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Use align_method in comp_method_FRAME #23132
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.
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
Conversation
Hello @jbrockmendel! Thanks for submitting the PR.
|
@@ -1934,6 +1934,9 @@ def _comp_method_FRAME(cls, func, special): | |||
|
|||
@Appender('Wrapper for comparison method {name}'.format(name=op_name)) | |||
def f(self, other): | |||
|
|||
other = _align_method_FRAME(self, other, axis=None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line does all the work of changing broadcasting behavior.
i think was ok with this before, needs a rebase, @jorisvandenbossche |
Codecov Report
@@ Coverage Diff @@
## master #23132 +/- ##
==========================================
+ Coverage 92.19% 92.22% +0.03%
==========================================
Files 169 169
Lines 50956 50877 -79
==========================================
- Hits 46978 46922 -56
+ Misses 3978 3955 -23
Continue to review full report at Codecov.
|
@jorisvandenbossche focusing here instead of PeriodArray for a bit. Can you recap your concerns? |
@jorisvandenbossche gentle ping. You're the gate-keeper for this and #23113. |
would also merge master here |
lgtm. merging shorty. |
|
||
- operating against a 2-dimensional ``np.ndarray`` with either 1 row or 1 column will now broadcast the same way a ``np.ndarray`` would (:issue:`23000`). | ||
- a list or tuple with length matching the number of rows in the :class:`DataFrame` will now raise ``ValueError`` instead of operating column-by-column (:issue:`22880`. | ||
- a list or tuple with length matching the number of columns in the :class:`DataFrame` will now operate row-by-row instead of raising ``ValueError`` (:issue:`22880`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this case match numpy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
arr = np.arange(6).reshape(3, 2)
>>> arr == [0, 1]
array([[ True, True],
[False, False],
[False, False]])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok then
thanks! |
This reverts commit 1735f42.
@jreback I know I have not been responsive here (I have been rather busy with thoroughly looking at all the EA related stuff), but I would appreciate if you didn't merge this without me looking at it (I was the original reason this got reverted). There are other ways to reach out to me. |
this was blocking other things; you need to divide your time then |
Then at least mention it is blocking other PRs, then I can try to prioritize it |
you were pinged multiple times if u really want to block then put a review request |
Redux to #22880 @jorisvandenbossche
Closes #20090
Summary:
DataFrame comparison operations broadcasting is inconsistent with arithmetic operations broadcasting. This PR fixes that.