-
-
Notifications
You must be signed in to change notification settings - Fork 146
feat(series): #1098 arithmetic addition #1275
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
base: main
Are you sure you want to change the base?
feat(series): #1098 arithmetic addition #1275
Conversation
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.
Looks like great progress here!
3b3f899
to
bf79de1
Compare
tests/series/arithmetic/test_add.py
Outdated
"""Test pd.Series[Any] + Python native scalars""" | ||
i, f, c = 1, 1.0, 1j | ||
|
||
# check(assert_type(left + i, pd.Series), pd.Series) # why? |
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.
Hi @twoertwein @loicdiridollou , I am making just minimal changes in __add__
now, and trying to understand the behaviour of self: typing
.
May I ask why Series[Any] + int
now gives Series[int]
? From my point of view, what I added is that only Series[int] + int
gives Series[int]
, and Series[Any]
as the left operand is irrelevant. Thanks.
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.
I think for Any/unknown, the overload order matters, the int overload is now the first one.
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.
I believe I had a PR or issue where the overload order prevented me when I tried to remove TimestampSeries
- unfortuntely, I can't find the PR/issue.
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.
Hi @twoertwein , thanks for the reply. I have specified self: Series[int]
, so that for Series[Any]
the first overload should not be triggered.
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.
type checkers match the first matching overload, I believe Any/unknown matches anything/the first overload
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.
here is an example where we had issues in the past with the overload order with any/unknown: #571 (comment)
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.
I think now I understand it better. Is the current MR the direction to go? I just need to add combinations of overloads.
bf79de1
to
6b592c2
Compare
02c646d
to
8f43efc
Compare
This PR is motivated by #1273 (review).
Comprehensive plan (maybe in another PR)
Inspired by #1093, one can draft the following comprehensive plan for all possible combinations of the left arithmetic operand, the binary arithmetic operator and the right arithmetic operand.
Left operand
Series[Any]
Series[int]
Series[float]
Series[complex]
Binary arithmetic operators
Include their right-version and functional version. In
add
we give a full example. We follow the official documentation.__add__
,__radd__
,add
,radd
__truediv__
__floordiv__
__pow__
__mod__
__mul__
__sub__
Right operand
Scalar
Sequence
numpy
arraysSeries
Draft of a plan
add
.Checklist
assert_type()
to assert the type of any return value