-
-
Notifications
You must be signed in to change notification settings - Fork 146
refactor: #718 also 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
base: main
Are you sure you want to change the base?
refactor: #718 also 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).
@@ -2379,4 +2339,4 @@ class OffsetSeries(Series[BaseOffset]): | |||
class IntervalSeries(Series[Interval[_OrderableT]], Generic[_OrderableT]): | |||
@property | |||
def array(self) -> IntervalArray: ... | |||
def diff(self, periods: int = ...) -> Never: ... | |||
def diff(self, periods: int = ...) -> Never: ... # pyrefly: ignore |
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.
Additional ignore, potential issue
@@ -1601,7 +1591,7 @@ def test_series_min_max_sub_axis() -> None: | |||
sd = s1 / s2 | |||
check(assert_type(sa, pd.Series), pd.Series) | |||
check(assert_type(ss, pd.Series), pd.Series) | |||
check(assert_type(sm, pd.Series), pd.Series) | |||
check(assert_type(sm, pd.Series), pd.Series) # type: ignore[assert-type] |
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.
Additional ignore, maybe mypy is not very clever here
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.
what does mypy infer?
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.
mypy
says the return type is 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.
Related: #1274 (comment)
1 / series | ||
[1] / series | ||
1 // series | ||
[1] // series |
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 had to remove the ignores, not sure if acceptable
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.
could potentially add -> Never
overloads?
tests/test_timefuncs.py
Outdated
ssum: pd.Series = s1 + s2 | ||
ts = pd.Timestamp("2022-06-30") | ||
tsum: pd.Series = s1 + ts # type: ignore[operator] # pyright: ignore[reportOperatorIssue] | ||
tsum: pd.Series = s1 + ts |
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 had to remove the ignores, not sure if acceptable
tests/test_timefuncs.py
Outdated
r1 = s1 * td # pyright: ignore[reportOperatorIssue] | ||
r2 = s1 / td # pyright: ignore[reportOperatorIssue] | ||
r3 = s3 * td # pyright: ignore[reportOperatorIssue] |
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 had to remove the ignores, not sure if acceptable
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.
_nonseries_int: TypeAlias = int | np_ndarray_anyint | Sequence[int] | ||
|
||
_T_INT = TypeVar("_T_INT", bound=int) | ||
_T_STAMP_AND_DELTA = TypeVar("_T_STAMP_AND_DELTA", bound=Timestamp | Timedelta) |
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.
might be nice to use consistent upper/lower-case across type-aliases/var: _int_t
or _intT
(similar to NDFrameT)
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 not very consistent everywhere, e.g.
_T_INT = TypeVar("_T_INT", bound=Series[int] | Index[int]) |
@@ -1601,7 +1591,7 @@ def test_series_min_max_sub_axis() -> None: | |||
sd = s1 / s2 | |||
check(assert_type(sa, pd.Series), pd.Series) | |||
check(assert_type(ss, pd.Series), pd.Series) | |||
check(assert_type(sm, pd.Series), pd.Series) | |||
check(assert_type(sm, pd.Series), pd.Series) # type: ignore[assert-type] |
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.
what does mypy infer?
1 / series | ||
[1] / series | ||
1 // series | ||
[1] // series |
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.
could potentially add -> Never
overloads?
5916fe1
to
3a27b71
Compare
3a27b71
to
dfd0dc5
Compare
dfd0dc5
to
6caa0c8
Compare
assert_type()
to assert the type of any return value