-
-
Notifications
You must be signed in to change notification settings - Fork 150
refactor(series)!: ⏱️ drop TimedeltaSeries #1273
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
refactor(series)!: ⏱️ drop TimedeltaSeries #1273
Conversation
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.
Overall this is a lot of great work, I have taken a crack at it and was quickly under water with the amount of changes needed.
I believe the __add__
issue that you have seems similar to what I am experiencing with #1098 and you may have the same problem with __sub__
and __truediv__
but you will be able to confirm.
I think we can't let those slip in the current version. Maybe the option is to keep the current TimestampSeries definition as a middle layer and start supporting the Series[Timestamp]
along the way (not sure if this is possible).
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.
Thank you for working on this!
(It might be too late for that, but it could be nice to split timedelta and timestamp into separate MRs.)
Overload order can be important when we have Series[Any]
or Series[unknown]
. After you get feedback from @Dr-Irv, it might be nice to add new tests for overloads that might have changed their any/unknown behavior.
5916fe1
to
3a27b71
Compare
3a27b71
to
dfd0dc5
Compare
6caa0c8
to
333eed7
Compare
333eed7
to
3df8ea0
Compare
So with this being dependent on #1274, I won't review this until that one is accepted and merged in. |
* refactor: only drop TimestampSeries #1273 (review) * fix(comment): https://github.com/pandas-dev/pandas-stubs/pull/1274/files#r2229555145 * less mypy ignore * fix: even less mypy ignores * refactor(comment): https://github.com/pandas-dev/pandas-stubs/pull/1274/files#r2229550572 * feat: sub * fix(comment): https://github.com/pandas-dev/pandas-stubs/pull/1274/files#r2229581983 * fix: pyrefly * fix(comment): https://github.com/pandas-dev/pandas-stubs/pull/1274/files#r2229580369 * fix(comment): #1274 (comment) * fix(comment): #1274 (comment) * fix(comment): #1274 (comment) * fix: reduce ignore * refactor: explain a temporary failure * fix(comment): #1274 (comment) * feat: arithmetic sub * fix(comment): #1312 (comment) * feat(comment): #1312 (comment) * fix(comment): completeness #1312 (review) * feat(series): arithmetic mul * fix: type asserts * feat: tests for addition * fix(mypy): attempt for python > 310 * feat: tests for subtraction * fix: pyright * fix(comment): #1274 (comment) * refactor: revert unused design * feat: progressive philosophy * fix(pytest): remove invalid tests * fix: comment * fix(comment): https://github.com/pandas-dev/pandas-stubs/pull/1274/files/f855f8655fcbc0c13879f404926034621641bc58#r2291133305 * fix(comment): #1274 (comment) * chore(philosophy): first draft * fix(pyright): median * fix: comment * fix(comment): #1274 (review) * fix(comment): test_sub * fix: typo
dbe65e4
to
89dd2f2
Compare
/pandas_nightly @Dr-Irv I've finished it from my point of view. There are a lot of changes. How do we proceed? I also added |
I'll review it and we'll take it from there. |
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.
This is looking pretty good. I would have preferred the changes for supporting floordiv
and truediv
, etc., that don't depend on Timedelta
to be in a separate PR, but given how close this is to working, I don't want to add extra work for you.
Can you create an issue in pandas-stubs
to create tests for arithmetic for TimedeltaIndex
and DatetimeIndex
, which should mirror the tests for pd.Series[Timedelta]
and pd.Series[Timestamp]
, respecitvely?
pandas-stubs/core/series.pyi
Outdated
numeric_only: _bool = False, | ||
**kwargs: Any, | ||
) -> float: ... | ||
) -> S1: ... |
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.
This isn't entirely correct, because pd.Series([1,2,3,4]).median()
returns a float
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.
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.
Only a few things
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.
figured out the to_numpy
issue with mypy
.
Would rather have median
, mean
and std
return float
on untyped Series
as it is more user friendly that way.
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 major accomplishment!
@cmp0xff I have just released v2.3.2.250926 so we'll see what feedback we will get |
TimestampSeries
,TimedeltaSeries
, etc. can be removed #718PeriodSeries
,OffsetSeries
,IntervalSeries
assert_type()
to assert the type of any return value