Skip to content

feat(arithmetic): ✖️ multiplication #1397

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 19 commits into from
Oct 6, 2025
Merged

feat(arithmetic): ✖️ multiplication #1397

merged 19 commits into from
Oct 6, 2025

Conversation

cmp0xff
Copy link
Contributor

@cmp0xff cmp0xff commented Sep 30, 2025 •
edited
Loading

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Oct 1, 2025

Will await your ping for the review.

@cmp0xff cmp0xff force-pushed the feature/cmp0xff/arithmetic-mul branch from f1acc37 to 19191a4 Compare October 1, 2025 20:22
@cmp0xff cmp0xff force-pushed the feature/cmp0xff/arithmetic-mul branch from 19191a4 to 9c802b4 Compare October 1, 2025 21:22
cmp0xff
cmp0xff commented Oct 1, 2025
View reviewed changes
Copy link
Contributor Author

@cmp0xff cmp0xff left a comment

Choose a reason for hiding this comment

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

@Dr-Irv this #1397 is ready to be reviewed, thanks.

@cmp0xff cmp0xff marked this pull request as ready for review October 1, 2025 21:37
@cmp0xff
Copy link
Contributor Author

cmp0xff commented Oct 4, 2025

@Dr-Irv this #1397 is ready to be reviewed, thanks.

I will mention @Dr-Irv again just in case you missed this pull request. Thanks!

@cmp0xff cmp0xff requested a review from Dr-Irv October 4, 2025 17:16
Dr-Irv
Dr-Irv requested changes Oct 4, 2025
View reviewed changes
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.

Should tests be added that make sure that if a str is used in multiplication, we catch that as invalid? And that both Series[str] and Index[str] with multiplication are caught as well?

This is in pretty good shape. I think you are missing some tests on Index[Any] with multiplication.

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Oct 4, 2025

@Dr-Irv this #1397 is ready to be reviewed, thanks.

I will mention @Dr-Irv again just in case you missed this pull request. Thanks!

I had to wait until today when I had time to review this large of a PR

cmp0xff
cmp0xff commented Oct 4, 2025
View reviewed changes
Copy link
Contributor Author

@cmp0xff cmp0xff left a comment

Choose a reason for hiding this comment

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

Tests for Series[str] * num and Index[str] * num added in 6cf48fc

@cmp0xff cmp0xff requested a review from Dr-Irv October 4, 2025 19:26
@cmp0xff
Copy link
Contributor Author

cmp0xff commented Oct 5, 2025

/pyright_strict

@cmp0xff
Copy link
Contributor Author

cmp0xff commented Oct 5, 2025

/pandas_nightly

@cmp0xff
Copy link
Contributor Author

cmp0xff commented Oct 5, 2025

/mypy_nightly

@cmp0xff
Copy link
Contributor Author

cmp0xff commented Oct 5, 2025

In pandas_nightly (see the pipeline run):

  • timedelta64 and str multiplying bool are forbidden. Shall we forbid them now?
  • check(assert_type(s.info(show_counts=False), None), type(None)) is broken, I think we should report it.

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Oct 5, 2025

  • check(assert_type(s.info(show_counts=False), None), type(None)) is broken, I think we should report it.

pandas-dev/pandas#62590

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Oct 5, 2025

  • timedelta64 and str multiplying bool are forbidden. Shall we forbid them now?

Yes

Dr-Irv
Dr-Irv requested changes Oct 5, 2025
View reviewed changes
@cmp0xff
Copy link
Contributor Author

cmp0xff commented Oct 5, 2025

/pandas_nightly

cmp0xff
cmp0xff commented Oct 5, 2025
View reviewed changes
Copy link
Contributor Author

@cmp0xff cmp0xff left a comment

Choose a reason for hiding this comment

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

  • pandas nightly: also fixed with fb97790
  • multiplying with "str": caught with test_mul_str_py_str (both for Series and Index)

@cmp0xff cmp0xff requested a review from Dr-Irv October 5, 2025 17:40
Dr-Irv
Dr-Irv requested changes Oct 5, 2025
View reviewed changes
cmp0xff
cmp0xff commented Oct 6, 2025
View reviewed changes
Copy link
Contributor Author

@cmp0xff cmp0xff left a comment

Choose a reason for hiding this comment

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

/pandas_nightly

@cmp0xff
Copy link
Contributor Author

cmp0xff commented Oct 6, 2025

/pandas_nightly

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Oct 6, 2025

@cmp0xff I think this is good to go. Let me know if there are any other changes you intend to make.

@cmp0xff cmp0xff requested a review from Dr-Irv October 6, 2025 20:36
@cmp0xff
Copy link
Contributor Author

cmp0xff commented Oct 6, 2025

@cmp0xff I think this is good to go. Let me know if there are any other changes you intend to make.

Hi @Dr-Irv I forgot to click "request review". There is nothing I want to change further.

Dr-Irv
Dr-Irv approved these changes Oct 6, 2025
View reviewed changes
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 c73ec4c into pandas-dev:main Oct 6, 2025
14 checks passed
@cmp0xff cmp0xff deleted the feature/cmp0xff/arithmetic-mul branch October 6, 2025 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dr-Irv Dr-Irv Dr-Irv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cmp0xff @Dr-Irv