Skip to content

Conversation

cmp0xff
Copy link
Contributor

@cmp0xff cmp0xff commented Sep 30, 2025

@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
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.

Comment on lines 292 to 298
def test_fail_on_adding_two_timestamps() -> None:
s1 = pd.Series(pd.to_datetime(["2022-05-01", "2022-06-01"]))
s2 = pd.Series(pd.to_datetime(["2022-05-15", "2022-06-15"]))
if TYPE_CHECKING_INVALID_USAGE:
ssum = s1 + s2 # type: ignore[operator] # pyright: ignore[reportOperatorIssue]
ts = pd.Timestamp("2022-06-30")
tsum = s1 + ts # type: ignore[operator] # pyright: ignore[reportOperatorIssue]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now in tests/series/arithmetic/timestamp/test_add.py

std = (df["x"] < df["y"]) * pd.Timedelta(10, "minutes")
check(assert_type(std, "pd.Series[pd.Timedelta]"), pd.Series, pd.Timedelta)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tests/series/arithmetic/bool/test_mul.py

r3 = td + ts
check(assert_type(r3, "pd.Series[pd.Timestamp]"), pd.Series, pd.Timestamp)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tests/series/arithmetic/timedelta/test_add.py

@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
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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why you have an empty file here, although I would assume some tests are necessary???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in c8e0105

Comment on lines +20 to +24
def left() -> "pd.Index[pd.Timedelta]":
"""left operand"""
# pandas-dev/pandas#62524
lo = pd.Index([1]) * [timedelta(seconds=1)] # left operand
return check(assert_type(lo, "pd.Index[pd.Timedelta]"), pd.Index, timedelta)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we should be testing this. pd.Index[Timedelta] should only hold Timedelta objects, not python timedelta objects. pd.Index([timedelta(seconds=1)]) creates this: TimedeltaIndex(['0 days 00:00:01'], dtype='timedelta64[ns]', freq=None) and that's the normal supported usage of timedelta in an index.

I'd even question whether we should have any operations supporting pd.Index[pd.Timedelta], because that really is a TimedeltaIndex .

Actually, thinking about it more, I don't think we should have Index[Timedelta] or Index[Timestamp] in the stubs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is in pandas-dev/pandas#62524, where I found out that pandas indeed creates Index[Timedelta] when one does pd.Index([1]) * [timedelta(days=1)], which is quite unintuitive.

@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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in c8e0105

Comment on lines +20 to +24
def left() -> "pd.Index[pd.Timedelta]":
"""left operand"""
# pandas-dev/pandas#62524
lo = pd.Index([1]) * [timedelta(seconds=1)] # left operand
return check(assert_type(lo, "pd.Index[pd.Timedelta]"), pd.Index, timedelta)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is in pandas-dev/pandas#62524, where I found out that pandas indeed creates Index[Timedelta] when one does pd.Index([1]) * [timedelta(days=1)], which is quite unintuitive.

@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.

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