feat: Allow DataFrame binary ops to align on either axis and with loc… #544
Conversation
product-auto-label
bot
added
size: m
TrevorBergeron
requested review from
milkshakeiii
and removed request for
Genesis929
TrevorBergeron
force-pushed
the
index_align
branch
from
bf5f61a to
f9e99a2
Compare
| "bf_series", | ||
| ], | ||
| ) | ||
| def test_listlike_binop_axis_1(scalars_dfs, input): |
There was a problem hiding this comment.
Should we add axis=1 to some existing tests for various binary ops?
There was a problem hiding this comment.
axis=1 completely changes alignment so the objects would just fail to align on this axis for existing tests. I'm not too worried about the different ops breaking, the actual scalar op compilation doesn't change for axis=1, the big worry is alignment failing
There was a problem hiding this comment.
Oh, right, the existing objects wouldn't wouldn't work. Okay, sounds good.
bigframes/core/normalize.py
Outdated
| import bigframes.series as series | ||
|
|
||
|
|
||
| def normalize_to_bf_series(obj, default_index: index.Index) -> series.Series: |
There was a problem hiding this comment.
I would prefer just "to_bf_series" since to_blah seems like a common name for things that basically just convert, and "normalize" has so many possible meanings.
| elif isinstance(other, DataFrame): | ||
| return self._apply_dataframe_binop(other, op, how=how, reverse=reverse) | ||
| elif isinstance(other, pandas.DataFrame): |
There was a problem hiding this comment.
might need to add pandas.DataFrame to the type hint for other?
There was a problem hiding this comment.
For what purpose? Mypy is able to infer based on the isinstance checks.
There was a problem hiding this comment.
Oh, gotcha, but should we remove the other type hints then? Just for consistency/cleanliness.
…al objects.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕