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.
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
| ), | ||
| ) -> Series[Timedelta]: ... | ||
| @overload | ||
| def __sub__(self: Series[Period], other: Series[Period]) -> OffsetSeries: ... |
There was a problem hiding this comment.
TLDR:
- Use
-> Series[Offset] - Please also add
def __rsub__(self: Series[Period], other: Series[Period]) -> OffsetSeries: ...and the correspondingsubandrsub. - Would be great if
tests/series/period/test_sub.pycan 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.
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.
Dr-Irv
left a comment
There was a problem hiding this comment.
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.
| ), | ||
| ) -> Series[Timedelta]: ... | ||
| @overload | ||
| def __sub__(self: Series[Period], other: Series[Period]) -> OffsetSeries: ... |
There was a problem hiding this comment.
| 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. |
| ]: ... | ||
| @property | ||
| def dt(self) -> _dtDescriptor[S1]: ... | ||
| dt = _dtDescriptor[S1]() # type: ignore[misc] |
There was a problem hiding this comment.
@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.
Yes that is correct it works, I had missed the comment
assert_type()to assert the type of any return value