-
-
Notifications
You must be signed in to change notification settings - Fork 145
refactor: #718 only drop TimestampSeries #1274
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?
Conversation
check( | ||
assert_type(s5.dt.as_unit("s"), "pd.Series[pd.Timedelta]"), # type: ignore[assert-type] | ||
pd.Series, | ||
pd.Timedelta, | ||
) | ||
check( | ||
assert_type(s5.dt.as_unit("ms"), "pd.Series[pd.Timedelta]"), # type: ignore[assert-type] | ||
pd.Series, | ||
pd.Timedelta, | ||
) | ||
check( | ||
assert_type(s5.dt.as_unit("us"), "pd.Series[pd.Timedelta]"), # type: ignore[assert-type] | ||
pd.Series, | ||
pd.Timedelta, | ||
) | ||
check( | ||
assert_type(s5.dt.as_unit("ns"), "pd.Series[pd.Timedelta]"), # type: ignore[assert-type] | ||
pd.Series, | ||
pd.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.
These type: ignore
s will be removed in #1273
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.
need to not be there now
@overload | ||
def __add__( | ||
self: Series[Timestamp], other: _nonseries_timestamp | Series[Timestamp] | ||
) -> Never: ... |
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.
Disabling mypy from recognising Series[Any] + Series[Any] -> Series[Any]
in test_sum_get_add
and test_series_min_max_sub_axis
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 understand this comment.
check(assert_type(sa, pd.Series), pd.Series) | ||
check(assert_type(ss, pd.Series), pd.Series) | ||
check(assert_type(sa, pd.Series), pd.Series) # type: ignore[assert-type] | ||
check(assert_type(ss, 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.
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.
but you shouldn't have the ignores here. I'm really hesitant to accept this change in this PR without knowing that both PRs work together without this change.
check(assert_type(s + summer, pd.Series), pd.Series) # type: ignore[assert-type] | ||
check(assert_type(s + df["y"], pd.Series), pd.Series) # type: ignore[assert-type] | ||
check(assert_type(summer + summer, 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.
c81cd6e
to
d5e1089
Compare
d5e1089
to
41c7015
Compare
41c7015
to
abf9147
Compare
abf9147
to
cbbd372
Compare
@cmp0xff you have a number of PRs submitted while I was out on vacation for 2 weeks. Can you let me know which ones I should prioritize for review? |
Hi @Dr-Irv, I hope you had a nice vacation. My pull requests are categorised below. Each category is independent, but those in a higher position have a slightly higher priority in my opinion.
|
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 for doing this. It's a lot of good work.
Main thing - if I'm going to merge this PR, it needs to be in a state where we don't need the followup PR.
Basic rule - we don't put ignore
in the tests unless we are testing that the stubs should not accept something that is invalid. You have places where you have added ignore
in the tests and I won't merge that in (unless we know it is a bug in the type checker)
The above code (without the `reveal_type()` statements) will get a `Never` | ||
on the computation of `ssum` because it is | ||
inappropriate to add two series containing `Timestamp` values. The types will be |
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.
No, the above code will raise an Exception
at runtime - not Never
. Please revert this change.
@@ -46,19 +48,21 @@ ssum = s1 + s2 | |||
reveal_type(ssum) | |||
``` | |||
|
|||
The above code (without the `reveal_type()` statements) will raise an `Exception` on the computation of `ssum` because it is | |||
The above code (without the `reveal_type()` statements) will get a `Never` | |||
on the computation of `ssum` because it is | |||
inappropriate to add two series containing `Timestamp` values. The types will be | |||
revealed as follows: |
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.
revealed as follows: | |
revealed by `mypy` as follows: |
(I'm assuming this was using mypy
in your updated example)
check(assert_type(s + summer, pd.Series), pd.Series) # type: ignore[assert-type] | ||
check(assert_type(s + df["y"], pd.Series), pd.Series) # type: ignore[assert-type] | ||
check(assert_type(summer + summer, 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.
don't want to have ignore
in the tests. Fix the types to make this work.
@@ -3469,6 +3466,7 @@ def test_groupby_apply() -> None: | |||
df = pd.DataFrame({"col1": [1, 2, 3], "col2": [4, 5, 6]}) | |||
|
|||
def sum_mean(x: pd.DataFrame) -> float: | |||
x.sum() |
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.
remove
@@ -1264,9 +1268,13 @@ def test_timestamp_cmp() -> None: | |||
# DatetimeIndex, but the type checker detects it to be UnknownIndex. | |||
c_unknown_index = pd.DataFrame({"a": [1]}, index=c_datetimeindex).index | |||
c_np_ndarray_dt64 = np_dt64_arr | |||
c_series_dt64: TimestampSeries = pd.Series([1, 2, 3], dtype="datetime64[ns]") | |||
c_series_dt64 = pd.Series([1, 2, 3], dtype="datetime64[ns]") |
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.
can you add a check(assert_type(...
on c_series_dt64
to make sure we get a Series[Timestamp]
?
check( | ||
assert_type(s5.dt.as_unit("s"), "pd.Series[pd.Timedelta]"), # type: ignore[assert-type] | ||
pd.Series, | ||
pd.Timedelta, | ||
) | ||
check( | ||
assert_type(s5.dt.as_unit("ms"), "pd.Series[pd.Timedelta]"), # type: ignore[assert-type] | ||
pd.Series, | ||
pd.Timedelta, | ||
) | ||
check( | ||
assert_type(s5.dt.as_unit("us"), "pd.Series[pd.Timedelta]"), # type: ignore[assert-type] | ||
pd.Series, | ||
pd.Timedelta, | ||
) | ||
check( | ||
assert_type(s5.dt.as_unit("ns"), "pd.Series[pd.Timedelta]"), # type: ignore[assert-type] | ||
pd.Series, | ||
pd.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.
need to not be there now
check(assert_type((s3 - td), "TimedeltaSeries"), pd.Series, pd.Timedelta) | ||
check(assert_type((s3 + td), "TimedeltaSeries"), pd.Series, pd.Timedelta) | ||
check(assert_type((s3 / td), "pd.Series[float]"), pd.Series, float) | ||
if TYPE_CHECKING_INVALID_USAGE: | ||
r1 = s1 * td # type: ignore[operator] # pyright: ignore[reportOperatorIssue] | ||
r1 = s1 * td # type: ignore[var-annotated] |
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.
need to include a pyright
ignore
here as well
@overload | ||
def __add__( | ||
self: Series[Timestamp], other: _nonseries_timestamp | Series[Timestamp] | ||
) -> Never: ... |
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 understand this comment.
@overload | ||
def __add__(self: Series[Timedelta], other: Period) -> PeriodSeries: ... | ||
@overload | ||
def __add__( | ||
self: Series[Timedelta], other: _nonseries_timestamp | Series[Timestamp] | ||
) -> Series[Timestamp]: ... |
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.
Shouldn't anything referring to Series[Timedelta]
be methods in TimedeltaSeries
and then you fix that in the other PR?
@@ -1910,7 +2053,7 @@ class Series(IndexOpsMixin[S1], NDFrame): | |||
level: None = ..., | |||
numeric_only: _bool = ..., | |||
**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 has to remain float
. Ideally, we would return Never
for things like Series[str]
where mean is not defined.
TimestampSeries
,TimedeltaSeries
, etc. can be removed #718assert_type()
to assert the type of any return value