Skip to content

feat(series): add and truediv for bools #1314

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

Merged
merged 10 commits into from
Aug 13, 2025

Conversation

cmp0xff
Copy link
Contributor

@cmp0xff cmp0xff commented Aug 11, 2025

This is inspired by #1312 (comment) and a follow-up for #1311.

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

@cmp0xff cmp0xff marked this pull request as draft August 12, 2025 08:15
@cmp0xff cmp0xff marked this pull request as ready for review August 12, 2025 08:33
@cmp0xff cmp0xff marked this pull request as draft August 12, 2025 15:11
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.

Just have to add a few comments so we understand why the ignores are there in the tests.

I caught something when eyeballing the difference between __truediv__() and truediv() .

It's odd that truediv() works for mypy but __truediv__() does not.

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 see that you were able to remove the 2 ignores. Request a review when you've handled my comments from the previous review (2 of which you can now dismiss) and from this review.

@cmp0xff cmp0xff requested a review from Dr-Irv August 12, 2025 16:25
@cmp0xff cmp0xff marked this pull request as ready for review August 12, 2025 16:26
@cmp0xff
Copy link
Contributor Author

cmp0xff commented Aug 12, 2025

It's odd that truediv() works for mypy but truediv() does not.

Our test coverage used to be poorer than it is now. truediv was not tested to the same standard as __truediv__.

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 checked and __truediv__() and truediv() have the same patterns.

Then checked __rtruediv__() and rtruediv() and found discrepancies.

Probably should do the same for add() and radd() at some point, but that can be another PR

@overload
def __rtruediv__( # pyright: ignore[reportOverlappingOverload]
self: Series[bool],
other: float | Sequence[float] | np_ndarray_anyint | np_ndarray_float,
Copy link
Collaborator

Choose a reason for hiding this comment

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

rtruediv() has Series[int]andSeries[float]forother` - please reconcile

@@ -2255,65 +2326,87 @@ class Series(IndexOpsMixin[S1], NDFrame):
self: Series[float], other: _T_COMPLEX | Sequence[_T_COMPLEX]
Copy link
Collaborator

Choose a reason for hiding this comment

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

rtruediv() has Series[_T_COMPLEX] for other - please reconcile

@cmp0xff cmp0xff marked this pull request as draft August 13, 2025 07:23
@cmp0xff cmp0xff changed the title feat(series): truediv for bools feat(series): add and truediv for bools Aug 13, 2025
@cmp0xff cmp0xff marked this pull request as ready for review August 13, 2025 15:10
@cmp0xff cmp0xff requested a review from Dr-Irv August 13, 2025 15:10
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 for also checking on __radd__()/radd() . I've checked them all now, and found one that I commented on, and one that is missing but I couldn't write a comment. This one is missing from add():

    @overload
    def __add__(self, other: Series[Never]) -> Series: ...

Otherwise, it's all good

@cmp0xff cmp0xff requested a review from Dr-Irv August 13, 2025 20:05
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 for the nice contribution!

Might want to add some tests for things we don't want to allow, e.g.,

s = pd.Series(["abc", "def"])  # inferred as Series[str]
s / "456"

The latter division will fail, and we are detecting it via the type checks, but having tests for that would be a nice addition, for another PR. Having said that, eagerly awaiting your PRs for subtraction and multiplication!

@Dr-Irv Dr-Irv merged commit fe3b81a into pandas-dev:main Aug 13, 2025
13 checks passed
@cmp0xff cmp0xff deleted the feature/cmp0xff/bool-truediv branch August 14, 2025 07:50
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