Skip to content

Conversation

cmp0xff
Copy link
Contributor

@cmp0xff cmp0xff commented Aug 27, 2025

It seems that #1347 requires a series of changes, albeit not in the sense of a Pandas Series. This PR could be a good place to start.

  • Tests added: Please use assert_type() to assert the type of any return value

Copy link
Collaborator

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good.

I think you can also remove __add__() and __radd__() from IndexOpsMixin because only Index and Series use IndexOpsMixin as a subclass, and now the definitions will reside in class Series and class Index .

I was thinking there might be a way to just put all the operators in IndexOpsMixin, but the challenge there is that Series.__add__(Series) returns a Series at runtime, but Index.__add__(Series) also returns a Series at runtime, so by not allowing Series as a parameter to Index.__add__(), the type checker will revert to the proper overload for Series.__radd__()

@cmp0xff cmp0xff requested a review from Dr-Irv August 28, 2025 21:58
Copy link
Collaborator

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can also remove __add__() and __radd__() from IndexOpsMixin because only Index and Series use IndexOpsMixin as a subclass, and now the definitions will reside in class Series and class Index .

@cmp0xff cmp0xff marked this pull request as draft August 28, 2025 22:37
@cmp0xff
Copy link
Contributor Author

cmp0xff commented Aug 29, 2025

  • There is no __add__() or __radd__() in IndexOpsMixin
  • There are __add__() and __radd__() in OpsMixin, which I removed in 8a0dcbd. I feel like it is a good idea. There are arithmetic operations that break the pattern -> Self, e.g. Series[Timestamp] - Series[Timestamp]. Removing such definitions may help resolving the conflicts, which still persist in my effort to merge main into refactor: #718 only drop TimestampSeries #1274.

@cmp0xff cmp0xff marked this pull request as ready for review August 29, 2025 00:58
@cmp0xff cmp0xff requested a review from Dr-Irv August 29, 2025 00:59
@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Aug 29, 2025

  • There is no __add__() or __radd__() in IndexOpsMixin
  • There are __add__() and __radd__() in OpsMixin, which I removed in 8a0dcbd. I feel like it is a good idea. There are arithmetic operations that break the pattern -> Self, e.g. Series[Timestamp] - Series[Timestamp]. Removing such definitions may help resolving the conflicts, which still persist in my effort to merge main into refactor: #718 only drop TimestampSeries #1274.

My mistake - I meant OpsMixin

Copy link
Collaborator

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @cmp0xff

@Dr-Irv Dr-Irv merged commit 58d59f2 into pandas-dev:main Aug 29, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants