-
-
Notifications
You must be signed in to change notification settings - Fork 150
GH1379 Remove PeriodSeries #1386
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
GH1379 Remove PeriodSeries #1386
Conversation
Weird places where this fails is the left_i = pd.DataFrame({"a": [1, 2, 3]})["a"]
a = pd.DataFrame({"a": [1, 2, 3]})["a"]
b = pd.Series([True, False, True])
i = pd.Series([2, 3, 5])
f = pd.Series([1.0, 2.0, 3.0])
c = pd.Series([1.1j, 2.2j, 4.1j])
check(assert_type(left_i - a, pd.Series), pd.Series) Gives us: |
@overload | ||
def diff(self: Series[_bool], periods: int = ...) -> Series[type[object]]: ... # type: ignore[overload-overlap] | ||
@overload | ||
def diff(self: Series[complex], periods: int = ...) -> Series[complex]: ... # type: ignore[overload-overlap] | ||
@overload | ||
def diff(self: Series[bytes], periods: int = ...) -> Never: ... | ||
@overload | ||
def diff(self: Series[type], periods: int = ...) -> Never: ... | ||
@overload | ||
def diff(self: Series[_str], periods: int = ...) -> Never: ... | ||
@overload | ||
def diff(self: Series[Timestamp], periods: int = ...) -> Series[Timedelta]: ... # type: ignore[overload-overlap] | ||
@overload | ||
def diff(self: Series[Timedelta], periods: int = ...) -> Series[Timedelta]: ... # type: ignore[overload-overlap] | ||
@overload | ||
def diff(self: Series[Period], periods: int = ...) -> OffsetSeries: ... # type: ignore[overload-overlap] | ||
@overload |
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 try the following idea to simplify, if you want.
class SupportsSelfSub(Protocol[_T_co]):
def __sub__(self, x: Self, /) -> _T_co: ...
And
def diff(self: SupportsGetItem[Scalar, SupportsSelfSub[S1_CO]], period: int = ...) -> Series[S1_CO]: ...
I've first seen the idea in #1362 and it worked again in #1374
pandas-stubs/core/series.pyi
Outdated
), | ||
) -> Series[Timedelta]: ... | ||
@overload | ||
def __sub__(self: Series[Period], other: Series[Period]) -> OffsetSeries: ... |
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.
TLDR:
- Use
-> Series[Offset]
- Please also add
def __rsub__(self: Series[Period], other: Series[Period]) -> OffsetSeries: ...
and the correspondingsub
andrsub
. - Would be great if
tests/series/period/test_sub.py
can be added
This one is the cause of #1386 (comment). I'll try to give my reasoning.
Note that self: Series[Period]
implies it also accepts self: Series[Any]
. The same applies to other: Series[Period]
. mypy
checks if the existing rules are "compatible" with the current result. Existing rules must have given Series[...]
, whereas the current result is OffsetSeries
. mypy
thinks this is _in_compatible, hence gives Any
.
That pyright
does not give Any
, could be a pyright
bug. See microsoft/pyright#10924
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.
Definitely changing the return type to Series[BaseOffset]
fixes that mypy
problem. But there is a problem with test_timefuncs.py
and PeriodProperties
and pyright
that I am trying to figure out.
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 got the test_timefuncs.py
to work by doing this change in series.pyi
:
dt = _dtDescriptor[S1]() # type: ignore[misc]
and removing the @property
and def dt
for Series
Also, pre-commit is failing, so make sure to update your local env before pushing.
pandas-stubs/core/series.pyi
Outdated
), | ||
) -> Series[Timedelta]: ... | ||
@overload | ||
def __sub__(self: Series[Period], other: Series[Period]) -> OffsetSeries: ... |
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.
def __sub__(self: Series[Period], other: Series[Period]) -> OffsetSeries: ... | |
def __sub__(self: Series[Period], other: Series[Period]) -> Series[BaseOffset]: ... |
so this is the same changes as I have been doing in #1385. Maybe @loicdiridollou can manually cherry-pick 🍒⛏ the relevant parts to reduce rebasing. In the end, the |
Thanks for the suggestion, I tried to add some tests into a new file according to @cmp0xff suggestion, we may be able to consolidate better in the future but this is a start. |
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.
pandas-stubs/core/series.pyi
Outdated
]: ... | ||
@property | ||
def dt(self) -> _dtDescriptor[S1]: ... | ||
dt = _dtDescriptor[S1]() # type: ignore[misc] |
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.
@cmp0xff indicated that the [S1]
here may not be necessary (and then the #type: ignore
isn't needed either). Can you test that?
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.
Yes that is correct it works, I had missed the 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 @loicdiridollou
assert_type()
to assert the type of any return value