Skip to content

Conversation

@cmp0xff
Copy link
Contributor

@cmp0xff cmp0xff commented Oct 28, 2025

@cmp0xff cmp0xff added Index Related to the Index class or subclasses Numeric Operations Arithmetic, Comparison, and Logical operations Series Series data structure labels Oct 28, 2025
@cmp0xff cmp0xff mentioned this pull request Oct 28, 2025
2 tasks
@cmp0xff cmp0xff force-pushed the feature/floordiv branch 4 times, most recently from 3df344a to 674b994 Compare October 31, 2025 11:02
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.

A few things:

  1. You're inconsistent when having timedelta divided by int where "divided" is either regular division or floor division, and timedelta is a single element or a sequence of some type. You have that as being not allowed in the stubs, but they work at runtime. Sometimes you do allow it, so you have to fix that.
  2. There are a few places where you say you can't detect via static typing, and I'm surprised at that.

Also, you are making changes here that are independent of supporting floordiv, namely:

  1. You changed the pyrefly override syntax
  2. You changed some imports from from pandas import Series to from pandas.core.series import Series

In the future, if you are making changes where the changes don't have to do with the main intent of the PR, make a separate PR for those changes. E.g., in the above case, you could do one for pyrefly and another to change the imports.

check(assert_type(c / left, "pd.Index[complex]"), pd.Index, np.complexfloating)
if TYPE_CHECKING_INVALID_USAGE:
_14 = s / left # type: ignore[operator] # pyright: ignore[reportOperatorIssue]
_15 = d / left # type: ignore[operator] # pyright: ignore[reportOperatorIssue]
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment as in indexes/test_floordiv.py . This does work:

>>> left = pd.Index([1.0, 2.0, 3.0])
>>> from datetime import timedelta
>>> d = [timedelta(seconds=s) for s in (1, 2, 3)]
>>> d / left
Index([0:00:01, 0:00:01, 0:00:01], dtype='object')

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 has been discussed in #1431 (review). The problem is that the result is Index[Timestamp], with element type Timestamp, but numpy dtype object. In #1431 we've decided to ban such usages.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case, can we just return Index without specifying what is inside? i.e., Index[Any] or just Index ?

if TYPE_CHECKING_INVALID_USAGE:
_13 = c // left # type: ignore[operator] # pyright: ignore[reportOperatorIssue]
_14 = s // left # type: ignore[operator] # pyright: ignore[reportOperatorIssue]
_15 = d // left # type: ignore[operator] # pyright: ignore[reportOperatorIssue]
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment - this should work

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 has been discussed in #1431 (review). The problem is that the result is Index[Timestamp], with element type Timestamp, but numpy dtype object. In #1431 we've decided to ban such usages.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case, can we just return Index without specifying what is inside? i.e., Index[Any] or just Index ?

if TYPE_CHECKING_INVALID_USAGE:
assert_type(c // left, Any)
assert_type(s // left, Any)
assert_type(d // left, "np.typing.NDArray[np.int64]")
Copy link
Collaborator

Choose a reason for hiding this comment

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

this works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check(assert_type(c / left, "pd.Index[complex]"), pd.Index, np.complexfloating)
if TYPE_CHECKING_INVALID_USAGE:
_14 = s / left # type: ignore[operator] # pyright: ignore[reportOperatorIssue]
_15 = d / left # type: ignore[operator] # pyright: ignore[reportOperatorIssue]
Copy link
Collaborator

Choose a reason for hiding this comment

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

this works

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 has been discussed in #1431 (review). The problem is that the result is Index[Timestamp], with element type Timestamp, but numpy dtype object. In #1431 we've decided to ban such usages.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case, can we just return Index without specifying what is inside? i.e., Index[Any] or just Index ?

if TYPE_CHECKING_INVALID_USAGE:
left_i.floordiv(c) # type: ignore[arg-type] # pyright: ignore[reportArgumentType,reportCallIssue]
left_i.floordiv(s) # type: ignore[arg-type] # pyright: ignore[reportArgumentType,reportCallIssue]
# left_i.floordiv(d) # This invalid one cannot be detected by static type checking
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is very surprising. You were able to detect that left_i // d is invalid, so you should be able to do this one.

if TYPE_CHECKING_INVALID_USAGE:
_03 = left_i // c # type: ignore[operator] # pyright: ignore[reportOperatorIssue]
_04 = left_i // s # type: ignore[operator] # pyright: ignore[reportOperatorIssue]
# _05 = left_i // d # This invalid one cannot be detected by static type checking
Copy link
Collaborator

Choose a reason for hiding this comment

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

A bit surprising, but then I see that you didn't add s and d in the tests in tests/series/arithmetic/test_truediv.py, so you should add that and things should be the same between floordiv and truediv

if TYPE_CHECKING_INVALID_USAGE:
left_i.floordiv(c) # type: ignore[arg-type] # pyright: ignore[reportArgumentType,reportCallIssue]
left_i.floordiv(s) # type: ignore[arg-type] # pyright: ignore[reportArgumentType,reportCallIssue]
# left_i.floordiv(d) # This invalid one cannot be detected by static type checking
Copy link
Collaborator

Choose a reason for hiding this comment

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

surprised at this

if TYPE_CHECKING_INVALID_USAGE:
_03 = left_i // c # type: ignore[operator] # pyright: ignore[reportOperatorIssue]
_04 = left_i // s # type: ignore[operator] # pyright: ignore[reportOperatorIssue]
# _05 = left_i // d # This invalid one cannot be detected by static type checking
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment

if TYPE_CHECKING_INVALID_USAGE:
left_i.floordiv(c) # type: ignore[arg-type] # pyright: ignore[reportArgumentType,reportCallIssue]
left_i.floordiv(s) # type: ignore[arg-type] # pyright: ignore[reportArgumentType,reportCallIssue]
# left_i.floordiv(d) # This invalid one cannot be detected by static type checking
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment

@cmp0xff cmp0xff marked this pull request as draft November 4, 2025 22:33
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.

1. You're inconsistent when having `timedelta` divided by `int` where "divided" is either regular division or floor division, and `timedelta` is a single element or a sequence of some type.  You have that as being not allowed in the stubs, but they work at runtime.  Sometimes you do allow it, so you have to fix that.

When the runtime result has the correct dtype, this PR should give type hints. When the operation crashes at runtime or gives object as dtype, this PR should show error. This was discussed in e.g. #1431 (comment), where Index[Timedelta] (which has dtype object) is banned.

2. There are a few places where you say you can't detect via static typing, and I'm surprised at that.

Still working on them. Will ping when finished.

1. You changed the `pyrefly` override syntax

Will do separately

2. You changed some imports from `from pandas import Series` to `from pandas.core.series import Series`

Will do separately.

check(assert_type(c / left, "pd.Index[complex]"), pd.Index, np.complexfloating)
if TYPE_CHECKING_INVALID_USAGE:
_14 = s / left # type: ignore[operator] # pyright: ignore[reportOperatorIssue]
_15 = d / left # 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.

This has been discussed in #1431 (review). The problem is that the result is Index[Timestamp], with element type Timestamp, but numpy dtype object. In #1431 we've decided to ban such usages.

if TYPE_CHECKING_INVALID_USAGE:
_13 = c // left # type: ignore[operator] # pyright: ignore[reportOperatorIssue]
_14 = s // left # type: ignore[operator] # pyright: ignore[reportOperatorIssue]
_15 = d // left # 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.

This has been discussed in #1431 (review). The problem is that the result is Index[Timestamp], with element type Timestamp, but numpy dtype object. In #1431 we've decided to ban such usages.

if TYPE_CHECKING_INVALID_USAGE:
assert_type(c // left, Any)
assert_type(s // left, Any)
assert_type(d // left, "np.typing.NDArray[np.int64]")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

check(assert_type(c / left, "pd.Index[complex]"), pd.Index, np.complexfloating)
if TYPE_CHECKING_INVALID_USAGE:
_14 = s / left # type: ignore[operator] # pyright: ignore[reportOperatorIssue]
_15 = d / left # 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.

This has been discussed in #1431 (review). The problem is that the result is Index[Timestamp], with element type Timestamp, but numpy dtype object. In #1431 we've decided to ban such usages.

if TYPE_CHECKING_INVALID_USAGE:
_13 = c // left_i # type: ignore[operator] # pyright: ignore[reportOperatorIssue]
_14 = s // left_i # type: ignore[operator] # pyright: ignore[reportOperatorIssue]
_15 = d // left_i # 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.

The result is Series with dtype object, not timedelta64 as expected. From #1431 I guess we also ban it.

if TYPE_CHECKING_INVALID_USAGE:
assert_type(c // left_i, Any)
assert_type(s // left_i, Any)
assert_type(d // left_i, Any) # pyright: ignore[reportAssertTypeFailure]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The result is Series with dtype object, not timedelta64 as expected. From #1431 I guess we also ban it.

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Nov 5, 2025

When the runtime result has the correct dtype, this PR should give type hints. When the operation crashes at runtime or gives object as dtype, this PR should show error. This was discussed in e.g. #1431 (comment), where Index[Timedelta] (which has dtype object) is banned.

I think this situation is different. When we banned Index[Timedelta] when the dtype was object, my concern was that you were doing tests using that type, and we don't want to assume it could be created.

Here, this is about the results of the computation. If we know the result has object dtype, I think the stubs can return a result without a specified value of S1 (i.e., either Index or Index[Any]; Series or Series[Any])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Index Related to the Index class or subclasses Numeric Operations Arithmetic, Comparison, and Logical operations Series Series data structure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants