feat(series): #1098 arithmetic addition

[cmp0xff]

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 arrays
• Series

Draft of a plan

• We can have 4 ~ 5 test scripts for each left operand
• In each test script, we can have 7 test functions for each binary arithmetic operator
• In each test function, we test on all right operands. Do not forget the possible four variations, see the example of add.

Checklist

• Closes Addition of a Series[int] with a complex returns Series[unknown] #1098, as stated in the title.
• Tests added: Please use assert_type() to assert the type of any return value

[loicdiridollou]

Looks like great progress here!

[twoertwein]

Thank you @cmp0xff!

I approved but I will let @Dr-Irv review/merge when he is back as this is a larger MR. I think adding all these overloads is in the the spirit of what pandas-stubs has been doing (but it might also create more maintenance burden).

(Personally, I'm still hoping for something like this #820 (comment) which would be more generic, but not supported by today's type checkers/python type system)

[Dr-Irv]

I really like the improvement of the tests and your approach. Thanks much for that.

I also like that you test the operator + (via both __add__() and __radd__(), and Series.add() and Series.radd(). But the arguments types for other for __add__() and Series.add() should be the same, and the argument types for other for __radd__() and Series.radd() should be the same.

As suggested in the review, if you make those changes, you should be in good shape.

[Dr-Irv]

I really do not want to have ignore statements in the tests.

I think if you modify __radd__() as follows it might work:

def __radd__( self: Series[complex], other: complex | Sequence[complex] | np_ndarray_anyint | np_ndarray_float | np_ndarray_complex ) -> Series[complex]: ...

[cmp0xff]

As I suggested in the comment, it did not make a difference when I included np_ndarray_* in the typing for other. My guess is that pandas.Series.__radd__ has a lower priority than np.ndarray.__add__, which already gives Any.

[Dr-Irv]

So you're correct about the precedence.

Actually, I think np.ndarray.__add__() is type checking to return np.ndarray, so can you change the test to show that it does return np_ndarray_int or np_ndarray_float in the assert_type() statements, but that the runtime result is a Series.

Then add a comment indicating why the tests are this way because of the precedence.

[cmp0xff]

4f11405 except for the mypy bug mentioned in the comment for the untyped Series

[Dr-Irv]

same issue as above. Modify __radd__() to include the numpy arrays as the other argument.

[cmp0xff]

4f11405

[Dr-Irv]

same - modify __radd__()

[cmp0xff]

4f11405

[Dr-Irv]

same - modify __radd__()

[cmp0xff]

4f11405

[cmp0xff]

the arguments types for other for add() and Series.add() should be the same, and the argument types for other for radd() and Series.radd() should be the same.

With 2576640, arguments types are now

• the same for Series.__add__ and Series.add`
• almost the same for Series.__radd__ and Series.radd, except for np_ndarray_* are not added to the former case, because numpy has already defined the results to be of type Any.

[cmp0xff]

As I suggested in the comment, it did not make a difference when I included np_ndarray_* in the typing for other. My guess is that pandas.Series.__radd__ has a lower priority than np.ndarray.__add__, which already gives Any.

[Dr-Irv]

Can you make __radd__() and radd() have the same arguments, even though it doesn't do what we want?

Can you update the tests as indicated in the comment so that you don't have the # type: ignore, but you use assert_type() on the type that numpy is returning, and then check() on the runtime type (pd.Series)

Also, I'm finding it hard to see what you changed since you're original commit. Please don't force push your changes, but just push your latest commits.

[cmp0xff]

Can you make __radd__() and radd() have the same arguments, even though it doesn't do what we want?

They are almost the same now, except for __radd__s do not need Series in other, because they are governed by __add__ already.

Can you update the tests as indicated in the comment so that you don't have the # type: ignore, but you use assert_type() on the type that numpy is returning, and then check() on the runtime type (pd.Series)

Added, except for #1275 (comment)

Also, I'm finding it hard to see what you changed since you're original commit. Please don't force push your changes, but just push your latest commits.

I see, but the base branch had been updated and there were conflicts. What should I do instead of rebasing, which caused the forced-update?

[Dr-Irv]

I see, but the base branch had been updated and there were conflicts. What should I do instead of rebasing, which caused the forced-update?

git checkout mybranch git fetch upstream git merge upstream/main

If there are conflicts when you merge, fix them, and commit, and you should be good.

[Dr-Irv]

thanks @cmp0xff . Really nice work!