added np.timedelta64 for series arithmatic methods #432

ramvikrams · 2022-11-18T19:14:59Z

Closes Series arithmetic methods accepting a Timedelta should also accept a np.timedelta64 #423
Tests added: Please use assert_type() to assert the type of any return value

sorry sir had to close that draft pr because for some reason I was not able to generate a pr from that

Dr-Irv

You also have to modify the operators in TimedeltaSeries

Dr-Irv · 2022-11-18T19:20:06Z

tests/test_timefuncs.py

+def timedelta64_and_arithmatic_operator() -> None:
+    s1 = pd.Series(data=pd.date_range("1/1/2020", "2/1/2020"))
+    s2 = pd.Series(data=pd.date_range("1/1/2021", "2/1/2021"))
+    check(assert_type((s1 - np.timedelta64(1, "M")), np.timedelta64), np.timedelta64)


Just create a variable td = np.timedelta64(1, "M") and reuse it

Dr-Irv · 2022-11-18T19:21:52Z

tests/test_timefuncs.py

+    check(assert_type((s1 - np.timedelta64(1, "M")), np.timedelta64), np.timedelta64)
+    check(assert_type((s1 + np.timedelta64(1, "M")), np.timedelta64), np.timedelta64)
+    check(assert_type(((s1 * np.timedelta64(1, "M")), np.timedelta64), np.timedelta64))
+    check(assert_type(((s1 / np.timedelta64(1, "M")), np.timedelta64), np.timedelta64))


The results here are wrong, because s1 is a TimeStampSeries, so adding and subtracting a np.timedelta64 will produce the same. Have to try each one out locally and see what the right type is.

Sir it's worling fine with addition,substraction only problem is with multiplication,division any suggestions for that

Sir it's worling fine with addition,substraction only problem is with multiplication,division any suggestions for that

Doesn't seem that it is working. Look at the log files from the CI runs (or run locally) and you can then see the error messages and fix the code.

tests/test_timefuncs.py

ramvikrams · 2022-11-18T19:34:00Z

You also have to modify the operators in TimedeltaSeries

yes sir will do

ramvikrams · 2022-11-19T16:58:32Z

Done sir

Dr-Irv · 2022-11-19T23:57:32Z

pandas-stubs/core/series.pyi

    def __radd__(self, pther: Timestamp | TimestampSeries) -> TimestampSeries: ...  # type: ignore[override]
-    def __mul__(self, other: num) -> TimedeltaSeries: ...  # type: ignore[override]
+    def __mul__(self, other: num | np.timedelta64) -> TimedeltaSeries: ...  # type: ignore[override]


should not add np.timedelta64 to TimedeltaSeries.__mul__(), as you can't multiple a series of Timedelta by np.timedelta64

Dr-Irv · 2022-11-20T00:13:44Z

tests/test_timefuncs.py

+    s3 = s2 - s1
+    td = np.timedelta64(1, "M")
+    check(assert_type((s1 - td), TimestampSeries), np.timedelta64)
+    check(assert_type((s1 + td), pd.Series[Any]), np.timedelta64)


This is incorrect. s1 is TimestampSeries, so you need to add a stub for __add__() in TimestampSeries with arguments Timedelta | np.timedelta64. Might also need a @overload

Dr-Irv · 2022-11-20T00:16:32Z

tests/test_timefuncs.py

+    check(assert_type((s1 - td), TimestampSeries), np.timedelta64)
+    check(assert_type((s1 + td), pd.Series[Any]), np.timedelta64)
+    check(assert_type((s1 * td), TimedeltaSeries), np.timedelta64)
+    check(assert_type((s1 / td), pd.Series[float]), np.timedelta64)


For the above 4 checks, I am surprised they got past pytest, because the ehecks should be of the form check(assert_type((s1 - td), TimestampSeries), pd.Series, pd.Timestamp) (for example)

Dr-Irv · 2022-11-20T00:17:21Z

tests/test_timefuncs.py

+    check(assert_type((s1 / td), pd.Series[float]), np.timedelta64)
+    check(
+        assert_type((s3 - td), TimedeltaSeries),
+        pd.Series,


add pd.Timedelta to the last argument of check here

Dr-Irv · 2022-11-20T00:18:04Z

tests/test_timefuncs.py

+    check(
+        assert_type((s3 * td), TimedeltaSeries),
+        pd.Series,
+    )


for above 2 checks, add pd.Timedelta to the last argument of check

Dr-Irv · 2022-11-20T00:18:34Z

tests/test_timefuncs.py

+    check(
+        assert_type((s3 / td), pd.Series[float]),
+        pd.Series,
+    )


add float to last argument of check . (Might need to be np.float)

Dr-Irv

Please do poetry run poe test before pushing new changes to make sure things work locally

Dr-Irv · 2022-11-20T17:24:59Z

pandas-stubs/core/series.pyi

@@ -1743,6 +1743,8 @@ class TimestampSeries(Series[Timestamp]):
    # ignore needed because of mypy
    @property
    def dt(self) -> TimestampProperties: ...  # type: ignore[override]
+    @overload
+    def __add__(self, other: Timedelta | np.timedelta64) -> TimedeltaSeries: ...


result here of adding TimestampSeries plus a timedelta is TimestampSeries

sir here there is no TimestampSeries

Look at line 1742

Dr-Irv · 2022-11-20T17:27:57Z

tests/test_timefuncs.py

-    )
+    check(assert_type((s1 - td), TimestampSeries), pd.Series, pd.Timestamp)
+    check(assert_type((s1 + td), TimestampSeries), pd.Series, pd.Timestamp)
+    check(assert_type((s1 * td), TimedeltaSeries), pd.Series, pd.Timestamp)


multipying a TimestampSeries times a Timedelta is not allowed, so have to fix that.

In TimestampSeries, put in a method for __mul__() that takes the two timedelta arguments, and returns Never from typing_extensions

Change the test to assert the type, but do not do the check part.

Dr-Irv · 2022-11-20T17:28:31Z

tests/test_timefuncs.py

+    check(assert_type((s1 - td), TimestampSeries), pd.Series, pd.Timestamp)
+    check(assert_type((s1 + td), TimestampSeries), pd.Series, pd.Timestamp)
+    check(assert_type((s1 * td), TimedeltaSeries), pd.Series, pd.Timestamp)
+    check(assert_type((s1 / td), TimestampSeries), pd.Series, pd.Timestamp)


dividing a TimestampSEries by a Timedelta is not allowed, so have to fix that

In TimestampSeries, put in a method for __truediv__() that takes the two timedelta arguments, and returns Never from typing_extensions

Change the test to assert the type, but do not do the check part.

Dr-Irv · 2022-11-20T17:29:16Z

tests/test_timefuncs.py

+    check(assert_type((s1 / td), TimestampSeries), pd.Series, pd.Timestamp)
+    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), TimedeltaSeries), pd.Series, pd.Timedelta)


multiplying a TimedeltaSeries by a timedelta fails, so have to fix that

In TimedeltaSeries, put in a method for __mul__() that takes the two timedelta arguments, and returns Never from typing_extensions

Change the test to assert the type, but do not do the check part.

Sir in TimedeltaSeries should a __trudiv__() be added because it is showing errors

and sir __mul__() it is still showing errors after adding it.

Sir in TimedeltaSeries should a __trudiv__() be added because it is showing errors

Yes, you should add that. It is valid to divide TimedeltaSeries by Timedelta or np.timedelta64 to get a Series[float], and to divde by a float to get TimedeltaSeries

and sir __mul__() it is still showing errors after adding it.

Your job to fix them!

Sir just not able to fix this error: Name "__mul__" already defined on line 1761 [no-redef] could not find about it over internet also

You probably have __mul__() defined in there twice, with different arguments. You need to put a @overload before each def __mul__() statement

Sir still mypy hows the same error

Without seeing your code, I can't see what you are doing wrong. You can push and it will fail and I will tell you what you did wrong.

Dr-Irv · 2022-11-21T22:04:30Z

pandas-stubs/core/series.pyi

    def __mul__(self, other: num) -> TimedeltaSeries: ...  # type: ignore[override]
    def __sub__(  # type: ignore[override]
        self, other: Timedelta | TimedeltaSeries | TimedeltaIndex | np.timedelta64
    ) -> TimedeltaSeries: ...
+    def __truediv__(self, other: TimedeltaSeries | np.timedelta64) -> Series[float]: ...  # type: ignore[override]
+    @overload
+    def __mul__(self, other: TimestampSeries | np.timedelta64) -> Never: ...  # type: ignore[override]


you need to put all the definitions of the same methods that have overloads together. Move lines 1766-1767 to after line 1761

error: Signature of "__mul__" incompatible with supertype "Series" [override] this is the last mypy error it doesn't go if I add # type: ignore[override] from line number 1760

you need to put all the definitions of the same methods that have overloads together. Move lines 1766-1767 to after line 1761

done sir

error: Signature of "__mul__" incompatible with supertype "Series" [override] this is the last mypy error it doesn't go if I add # type: ignore[override] from line number 1760

It's OK to put in the # type: ignore[override] in this case.

error: Signature of "__mul__" incompatible with supertype "Series" [override] this is the last mypy error it doesn't go if I add # type: ignore[override] from line number 1760

It's OK to put in the # type: ignore[override] in this case.

then it shows this error error: Unused "type: ignore" comment

check the line numbers that the error is reported on

It is on line 1760 the initial error is error: Signature of "__mul__" incompatible with supertype "Series" [override] and after I add # type: ignore[override] the initial error still remains

Push your current code and I can take a look at the CI logs

Dr-Irv · 2022-11-22T15:07:14Z

pandas-stubs/core/series.pyi

+    @overload
+    def __mul__(self, other: TimestampSeries | np.timedelta64 | Timedelta | TimedeltaSeries) -> Never: ...
+    @overload
+    def __mul__(self, other: num) -> TimedeltaSeries: ... 


You need to add # type: ignore[override] on both lines 1761 and 1763, or maybe just one of them, or possibly on lines 1760 and/or 1762. You'll have to experiment.

Dr-Irv · 2022-11-22T16:04:47Z

tests/test_timefuncs.py

+    assert_type((s3 - td), TimedeltaSeries)  # type: ignore
+    assert_type((s3 + td), TimedeltaSeries)  # type: ignore
+    assert_type((s3 * td), NoReturn)  # type: ignore
+    assert_type((s3 / td), pd.Series[float])  # type: ignore


Do NOT put #type: ignore in the testing functions.

ramvikrams · 2022-11-22T16:48:42Z

Done sir, I think it's complete

Dr-Irv · 2022-11-22T17:04:12Z

pandas-stubs/core/series.pyi

@@ -1739,6 +1746,9 @@ class TimestampSeries(Series[Timestamp]):
    # ignore needed because of mypy
    @property
    def dt(self) -> TimestampProperties: ...  # type: ignore[override]
+    def __add__(self, other: TimedeltaSeries | np.timedelta64 | TimestampSeries | Timestamp) -> TimestampSeries: ...  # type: ignore[override]


Remove TimestampSeries | Timestamp here as you can't add two timestamps

Dr-Irv · 2022-11-22T19:55:20Z

tests/test_timefuncs.py

+    assert_type((s1 / td), NoReturn)  # pyright: ignore
+    assert_type((s3 - td), TimedeltaSeries)
+    assert_type((s3 + td), TimedeltaSeries)
+    assert_type((s3 * td), NoReturn)  # pyright: ignore


Can you do the following:

for the 4 cases where you are expecting TimestampSeries or TimedeltaSeries, use check(assert_type( operation, expected_type), expected_type, expected_subtype) where operation is the operation you are testing, expected_type is the type expected, and expected_subtype is pd.Timestamp or pd.Timedelta as appropriate

For the 3 cases where you have NoReturn, can you change that to Never , which you will need to import from typing_extensions

The # pyright: ignore is correct - pyright is doing the right thing here, but mypy is not

Sir regarding point 2 and 3 when I changed it to Never pyrigth shows error, mypy works ok and if we add the #type: ignore both work fine so should I add it.
Because in the pyright error it says "assert_type" mismatch: expected "Never" but received "Unknown"

Yes, you will still need the # pyright: ignore where you had it before

Dr-Irv · 2022-11-22T19:55:35Z

tests/test_timefuncs.py

+    assert_type((s3 - td), TimedeltaSeries)
+    assert_type((s3 + td), TimedeltaSeries)
+    assert_type((s3 * td), NoReturn)  # pyright: ignore
+    assert_type((s3 / td), pd.Series[float])


use check on this one too

Dr-Irv · 2022-11-22T22:19:44Z

pandas-stubs/core/series.pyi

+        self, other: TimedeltaSeries | np.timedelta64 | TimestampSeries
+    ) -> TimestampSeries: ...
+    @overload
+    def __add__(self, other: Timestamp) -> TimestampSeries: ...


You can't add a Timestamp to a TimestampSeries. Make the return type Never here.

We have issue in line number 266 of tests/time_funcs.py if we change this, it's pyright issue.

On that line, change # TODO both: ignore[operator] to # pyright: ignore

Dr-Irv · 2022-11-22T23:01:49Z

tests/test_timefuncs.py

@@ -1035,3 +1038,18 @@ def test_timedelta_range() -> None:
 def test_dateoffset_freqstr() -> None:
    offset = DateOffset(minutes=10)
    check(assert_type(offset.freqstr, str), str)
+
+
+def timedelta64_and_arithmatic_operator() -> None:


the name of any test function must begin with test_. If you make that change, you will see that the tests will fail. They were never getting tested.

You will have to change the tests as well. See example.

Sir even after adding the test_ the poe mypy test doesn't fail on my system

Do poe test to run all tests or poe pytest to just run pytest which is where the failures will occur

yes sir there were errors, I'll fix it then

Dr-Irv · 2022-11-22T23:03:14Z

tests/test_timefuncs.py

+    s2 = pd.Series(data=pd.date_range("1/1/2021", "2/1/2021"))
+    s3 = s2 - s1
+    td = np.timedelta64(1, "M")
+    check(assert_type((s1 - td), TimestampSeries), TimestampSeries, pd.Timestamp)


check(assert_type((s1 - td), "TimestampSeries"), pd.Series, pd.Timestamp)

All the tests should look like this. The type TimestampSeries isn't known at runtime, so you have to surround it with quotes.

you will have to do something similar for TimedeltaSeries and pd.Series[float].

Look elsewhere in the code to see examples

Should we also change thte left 3 tests because they cannot pass pytets

Good point. Move those 3 tests to the end and do

if TYPE_CHECKING_INVALID_USAGE: # test 1 here # test 2 here # test 3 here

you import TYPE_CHECKING_INVALID_USAGE from tests

Dr-Irv

thanks @ramvikrams

By the way, in the future, when you make commits, please make the commit message reflect the changes you make, not just use the word "update"

ramvikrams · 2022-11-23T16:27:53Z

thanks @ramvikrams

By the way, in the future, when you make commits, please make the commit message reflect the changes you make, not just use the word "update"

yes sir sure I will keep that in mind

Thank You very much sir

update

acd9ac8

Dr-Irv requested changes Nov 18, 2022

View reviewed changes

ramvikrams added 9 commits November 19, 2022 13:48

update

19a7df2

Update test_timefuncs.py

e06f583

Update test_timefuncs.py

fb9dc01

Update test_timefuncs.py

83aa4c0

Update test_timefuncs.py

9fc4c76

Update test_timefuncs.py

dda43a9

Update test_timefuncs.py

994c6db

Update test_timefuncs.py

c0ea3b0

Update test_timefuncs.py

2284670

Dr-Irv requested changes Nov 20, 2022

View reviewed changes

update

9e03f29

Dr-Irv requested changes Nov 20, 2022

View reviewed changes

update

258e32a

Dr-Irv reviewed Nov 21, 2022

View reviewed changes

ramvikrams added 3 commits November 22, 2022 17:37

update

a5ed64d

update

fee79d6

Update series.pyi

fd59d02

Dr-Irv reviewed Nov 22, 2022

View reviewed changes

ramvikrams added 2 commits November 22, 2022 21:21

Update series.pyi

31db112

Update test_timefuncs.py

f01d3c0

Dr-Irv reviewed Nov 22, 2022

View reviewed changes

Update test_timefuncs.py

fc41d33

Dr-Irv requested changes Nov 22, 2022

View reviewed changes

update

21992b8

Dr-Irv requested changes Nov 22, 2022

View reviewed changes

update

cb26631

Dr-Irv mentioned this pull request Nov 23, 2022

FEATURE: Start using Never as return types for disallowed operations #441

Closed

Update test_timefuncs.py

ca75099

Dr-Irv approved these changes Nov 23, 2022

View reviewed changes

Dr-Irv merged commit b7163c2 into pandas-dev:main Nov 23, 2022

added np.timedelta64 for series arithmatic methods #432

added np.timedelta64 for series arithmatic methods #432

Conversation

ramvikrams commented Nov 18, 2022

Dr-Irv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramvikrams commented Nov 18, 2022

ramvikrams commented Nov 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dr-Irv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramvikrams Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramvikrams Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramvikrams Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramvikrams commented Nov 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramvikrams Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dr-Irv left a comment

Choose a reason for hiding this comment

ramvikrams commented Nov 23, 2022 • edited Loading

ramvikrams Nov 21, 2022 •

edited

Loading

ramvikrams Nov 21, 2022 •

edited

Loading

ramvikrams Nov 22, 2022 •

edited

Loading

ramvikrams Nov 23, 2022 •

edited

Loading

ramvikrams commented Nov 23, 2022 •

edited

Loading