-
-
Notifications
You must be signed in to change notification settings - Fork 153
feat(arithmetic): 🪜 floordiv #1452
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
Conversation
3df344a to
674b994
Compare
674b994 to
dca5fa2
Compare
Dr-Irv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things:
- You're inconsistent when having
timedeltadivided byintwhere "divided" is either regular division or floor division, andtimedeltais 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. - 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:
- You changed the
pyreflyoverride syntax - You changed some imports from
from pandas import Seriestofrom 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.
cmp0xff
left a comment
There was a problem hiding this 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.
I think this situation is different. When we banned Here, this is about the results of the computation. If we know the result has |
Dr-Irv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty close. Only a few things left
| pd.Series, | ||
| ) | ||
| if TYPE_CHECKING_INVALID_USAGE: | ||
| assert_type(s / left_i, Any) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
surprised that pyright isn't detecting this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have further insights 😔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's on the numpy side. pyright sees the type of s as "ndarray[tuple[Any, ...], dtype[datetime64[date | int | None]]]" . numpy has this overload that matches:
@overload
def __truediv__(self: NDArray[Any], other: _ArrayLikeObject_co, /) -> Any: ...Can you add a comment that indicates that numpy is not picking up that dividing a numpy array by anything and link to this issue I just created? numpy/numpy#30173
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dr-Irv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One open conversation - just add a comment about why pyright isn't picking up that one case.
Dr-Irv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @cmp0xff
This PR adds
floordivforIndexandSeries.TimedeltaIndexandDatetimeIndex#1378