TYP: remove NaTType as possible result of Timestamp and Timedelta constructor #46171

Dr-Irv · 2022-02-27T21:00:33Z

While working on the Microsoft type stubs, I discovered this. With the current code, if you run pyright on the following with default settings for basic type checking (not using what we have in pyproject.toml):

import pandas as pd
print("version ", pd.__version__)
ts:pd.Timestamp =  pd.Timestamp("2022-02-27")
print(ts)

you get this:

c:\Code\pandas_dev\pandas\tstest.py
  c:\Code\pandas_dev\pandas\tstest.py:3:20 - error: Expression of type "Timestamp | NaTType" cannot be assigned to declared type "Timestamp"
    Type "Timestamp | NaTType" cannot be assigned to type "Timestamp"
      "NaTType" is incompatible with "Timestamp" (reportGeneralTypeIssues)

mypy doesn't pick this up. The real issue here is that you generally expect a Timestamp constructor to return a Timestamp, so by returning the Union, we are then forcing end users to cast the result of the constructor, which is pretty inconvenient.

So even though the constructor could return a NaTType, we shouldn't force users to code for that due to static type checking.

This PR just takes out the NaTType as a return type for the constructors.

This reverts commit 84b119f.

jbrockmendel · 2022-02-27T22:19:45Z

mypy doesn't pick this up

Aren't the "type: ignore[misc]"s that this removes there specifically because mypy does pick it up?

So even though the constructor could return a NaTType, we shouldn't force users to code for that due to static type checking.

How big a deal is this for users? I'm uncomfortable with intentionally making the annotations less accurate.

The ideal solution is upstream in the type checkers accurately reflecting the capabilites of __new__, right?

cc @simonjayhawkins

Dr-Irv · 2022-02-27T23:08:46Z

How big a deal is this for users? I'm uncomfortable with intentionally making the annotations less accurate.

IMHO, this is a big deal. Otherwise, the code above would have to be written as:

from typing import cast
import pandas as pd
print("version ", pd.__version__)
ts:pd.Timestamp =  cast(pd.Timestamp, pd.Timestamp("2022-02-27"))
print(ts)

There's a tradeoff here to consider between how much the static type checker picks up versus what errors would get picked up in runtime.

Right now, pyright integration with VS Code is so good that just using it with defaults picks up lots of potential code errors, and we don't want to ask users to code around it.

The ideal solution is upstream in the type checkers accurately reflecting the capabilites of __new__, right?

Actually, pyright does pick up the __new__ part correctly, which is why it sees the return type of the constructor as Timestamp | NaTType, which then forces the end user to do a cast. mypy on the other hand does the reverse - it doesn't see that __new__() is returning the Union type and then the user doesn't have to change their code in my example.

twoertwein · 2022-02-28T00:14:16Z

How about using overloads to either return Timestamp or NaTType? Probably cannot catch all cases when NaTType might be returned but at least the obvious ones.

Edit:

Or can NaTType inherit from both Timedelta and Timestamp?

Dr-Irv · 2022-02-28T05:29:38Z

I have another solution that seems to be working locally. Idea is as follows:

Change Timestamp to _Timestamp and Timedelta to _Timedelta
Have Timestamp inherit from both _Timestamp and NaTType
Have Timedelta inherit from both _Timedelta and NaTType
For the new Timestamp and Timedelta, define methods appropriately via @overload that do the right thing based on whether the type of the class is _Timestamp/_Timedelta or NaTType

I can push that if people want to take a look, but it's a more substantive change.

Alternatively, I could create a second PR with that approach. Comments welcome.

jbrockmendel · 2022-02-28T15:50:22Z

Have Timedelta inherit from both _Timedelta and NaTType

Would need to do some more gymnastics to avoid having Timedelta inherit from datetime.

-0.5 on the original. Making our annotations less accurate for the sake of allowing users to make their annotations less accurate just seems counter-productive.

#24983 might be a better long-term solution.

Dr-Irv · 2022-02-28T16:02:47Z

Have Timedelta inherit from both _Timedelta and NaTType

Would need to do some more gymnastics to avoid having Timedelta inherit from datetime.

-0.5 on the original. Making our annotations less accurate for the sake of allowing users to make their annotations less accurate just seems counter-productive.

#24983 might be a better long-term solution.

So I have something that works - it only affects typing, and essentially is treating NaTType as a mixin from a typing perspective.

Question is whether I put that up as a separate PR or push it here.

twoertwein · 2022-02-28T20:46:56Z

I cannot comment on whether the type annotations should purposefully be different from the implementation. How do the microsoft/virtus lab stubs handle it?

I understand that from an end-user perspective, the current situation is as bad as it is with read_csv (DataFrame or TextFileReader). Except that for read_csv we can more easily create overloads.

Since mypy does anyways not recognize that __new__ can return a Union, I don't see too much value in keeping the Union. At the same time, I still want to keep the Union since it can return different types (and pyright picks it up).

If Timestamp/Timedelta were very simply (take only one argument), I wouldn't mind the following as a middle ground:

# cases when it is guanteed to be NaTType
# Literal[float("NaN")] is probably not valid
def __new__(value: None | Literal[float("NaN")] | ...) -> NaTType: ...

# might be NaTType, but in most cases is Timestamp
def __new__(value: BaseOffset | ...) -> Timestamp: ...

# in case the argument is a union of the above
def __new__(value: BaseOffset | ... | None | ...) -> Timestamp | NaTType: ...

Dr-Irv · 2022-02-28T21:09:34Z

How do the microsoft/virtus lab stubs handle it?

Well, I'm working on the microsoft ones now. I copied over what was in pandas, and discovered this issue, and have submitted a PR there to do what I propose here.

As for Virtus Labs, their stubs do not indicate NATType as a valid result of the constructor of Timestamp or Timedelta .

If Timestamp/Timedelta were very simply (take only one argument), I wouldn't mind the following as a middle ground:

We could do something similar with the current __new__() stubs.

Here's a question - I think the only way pd.NaT could be returned as a result of calling Timestamp or Timedelta is if you passed something like None, np.nan, etc. Other overflows, improper strings, etc. raise an exception. Can someone confirm? So maybe your solution - to type it differently based on the argument is a way to go.

jbrockmendel · 2022-02-28T22:21:56Z

I think the only way pd.NaT could be returned as a result of calling Timestamp or Timedelta is if you passed something like None, np.nan, etc. Other overflows, improper strings, etc. raise an exception. Can someone confirm?

A near-complete list:

None, np.nan, NaT, np.datetime64("NaT"), "NaT", "nat", "NAT", "nan", "NaN", "NAN", NaT.value

Dr-Irv · 2022-02-28T23:55:34Z

If Timestamp/Timedelta were very simply (take only one argument), I wouldn't mind the following as a middle ground:

# cases when it is guanteed to be NaTType
# Literal[float("NaN")] is probably not valid
def __new__(value: None | Literal[float("NaN")] | ...) -> NaTType: ...

# might be NaTType, but in most cases is Timestamp
def __new__(value: BaseOffset | ...) -> Timestamp: ...

I played around with this, and I don't think this is possible. You can't have different __new__ signatures return different types and make mypy work. The type recognition system in mypy won't return two different types based on the constructor. It's still a constructor for the primary type.

I think to make things good for the users, we have to go with what is in this PR.

Side note: I tried something like this, and pyright gets it right, but mypy doesn't (think of A as _Timestamp, B as NaTType, and C as Timestamp :

from __future__ import annotations
from typing import overload


class A:
    pass


class B:
    pass


class C(A, B):
    @overload
    def __new__(cls, x: int) -> A:
        ...

    @overload
    def __new__(cls, x: str) -> B:
        ...

    def __new__(cls, x: int | str) -> C | A | B:
        if isinstance(x, int):
            return super(A, cls).__new__(cls)
        elif isinstance(x, str):
            return super(B, cls).__new__(cls)
        else:
            raise Exception("bad argument")


y = C(2)
z = C("hey")

reveal_type(y)
reveal_type(z)

pyright (with defaults) reports:

new2.py:34:13 - information: Type of "y" is "A"
new2.py:35:13 - information: Type of "z" is "B"

mypy (with defaults) reports:

new2.py:15: error: Incompatible return type for "__new__" (returns "A", but must return a subtype of "C")
new2.py:19: error: Incompatible return type for "__new__" (returns "B", but must return a subtype of "C")
new2.py:22: error: "__new__" must return a class instance (got "Union[C, A, B]")
new2.py:34: note: Revealed type is "new2.C"
new2.py:35: note: Revealed type is "new2.C"
Found 3 errors in 1 file (checked 1 source file)

twoertwein · 2022-03-01T19:19:14Z

While I would like to avoid mypy ignores comments, I think your approach is working well:

The current implementation already has a mypy ignore comment to account for the missing functionality of mypy. Would just need to add one for each overload.
It still works perfectly for pyright :)
People using mypy will not notice any change
People using pyright get the "most likely" type based on the overloads
edit: and as long as mypy doesn't add this functionality, it does not degrade type checking for the pandas CI

From my perspective it looks good. The question is what do we prefer: type annotations being easy to use ("most likely" overloads) or being as strict as possible (always returning the union). Hopefully, the implementation will change in the future so that we don't need to make these trade-offs.

jreback · 2022-03-02T01:21:06Z

pandas/_libs/tslibs/timestamps.pyi

@@ -56,7 +54,7 @@ class Timestamp(datetime):
        tzinfo: _tzinfo | None = ...,
        *,
        fold: int | None = ...,
-    ) -> _DatetimeT | NaTType: ...
+    ) -> _DatetimeT: ...


can you add a comment (maybe ref this issue) about the reasoning here.

done in next commit

jreback

pls rebase as well

jreback · 2022-03-06T23:06:04Z

pandas/io/formats/format.py

@@ -1767,16 +1767,13 @@ def _format_datetime64_dateonly(
    nat_rep: str = "NaT",
    date_format: str | None = None,
 ) -> str:
-    if x is NaT:
+    if x is NaT or isinstance(x, NaTType):


why is NaTType leaking here? (is this is a typing issue? e.g. maybe cast?)

I think it is better to just do if isinstance(x, NaTType) here - the typing makes it clear. Will be in next commit.

checking for both NaT and NaTType is redundant. the existing check should be marginally more performant than an isintance check.

The issue here is that x is of type NaTType | Timestamp, so if you just do x is NaT then mypy can't conclude that x is Timestamp when the condition is False, but if you do isinstance(x, NaTType), it can make that conclusion when the condition is False.

im aware. i dont think that justifies making the check redundant.

That's why I just changed it to if isinstance(x, NaTType) in e79d253

as commented elsewhere, -1 on this change.

jreback · 2022-03-06T23:06:41Z

pandas/io/sas/sas_xport.py

@@ -139,7 +140,7 @@
 """


-def _parse_date(datestr: str) -> datetime:
+def _parse_date(datestr: str) -> datetime | NaTType:


I thought we have a type that is a datetime scalar | NatType already?

probably not bc ATM NaTType subclasses datetime

jreback · 2022-03-06T23:06:56Z

pandas/tests/resample/test_datetime_index.py

@@ -1286,7 +1291,7 @@ def test_resample_consistency():
    tm.assert_series_equal(s10_2, rl)


-dates1 = [
+dates1: List[Union[datetime, NaTType]] = [


ahh this, should be a first class type (the Union)

create a type in next commit

Change test for NaT in io/formats/format.py

jreback · 2022-03-07T23:54:26Z

@jbrockmendel ok here?

jbrockmendel · 2022-03-08T00:04:50Z

I remain -0.5, not gonna make a fuss about it.

jreback · 2022-03-08T00:23:03Z

I remain -0.5, not gonna make a fuss about it.

thanks! point noted.

twoertwein · 2022-04-05T22:43:23Z

Given the current two-tier plan for typing (copy public API from MS stubs and then potentially later merging them with the inline annotations)

it might have been cleaner to have kept NaTType as part of the inline annotations, as they fulfil a slightly different purpose compared to the public type stubs.

[Personally, I hope that once the MS stubs are copied over, it doesn't take too long to merge a majority of them - might be optimistic.]

simonjayhawkins · 2022-04-06T10:32:22Z

pandas/io/formats/format.py

@@ -1767,16 +1767,13 @@ def _format_datetime64_dateonly(
    nat_rep: str = "NaT",
    date_format: str | None = None,
 ) -> str:
-    if x is NaT:
+    if isinstance(x, NaTType):


-1 on code changes to satisfy the type checker. We know that static type checker have issues with singletons and imo that was adequately noted in the removed comment adjacent to the ignore statement.

It may be nbd is some cases, but am always -1 on principle.

simonjayhawkins · 2022-04-06T10:40:55Z

pandas/_libs/tslibs/timestamps.pyi

+    ) -> _DatetimeT: ...
+    # GH 46171
+    # While Timestamp can return pd.NaT, having the constructor return
+    # a Union with NaTType makes things awkward for users of pandas


I would be inclined to return Any in the public api if there are genuine concerns rather that have the incorrect return type.

simonjayhawkins · 2022-04-06T10:49:10Z

So even though the constructor could return a NaTType, we shouldn't force users to code for that due to static type checking.

I disagree, users type checking their code could be doing it to ensure more robust code, which is a different requirement to intellisense and other linting productivity tools.

The types should be precise (or less precise such as Any if we have issues to resolve) but not wrong.

simonjayhawkins · 2022-04-06T10:50:37Z

given @twoertwein and @jbrockmendel comments, I think this one should be reverted.

simonjayhawkins · 2022-04-06T10:55:38Z

From my perspective it looks good. The question is what do we prefer: type annotations being easy to use ("most likely" overloads) or being as strict as possible (always returning the union). Hopefully, the implementation will change in the future so that we don't need to make these trade-offs.

are you aware of other projects that use a "most likely" approach?

twoertwein · 2022-04-06T12:45:57Z

are you aware of other projects that use a "most likely" approach?

Unfortunately, no. But I would be happy to make a compromise to avoid having two different type annotations (internal usage vs. public usage). While I'm not a fan of two type annotations, I agree that we should start with the MS stubs to have a smooth transition from a user-perspective.

simonjayhawkins · 2022-04-06T13:07:49Z

I agree that we should start with the MS stubs to have a smooth transition from a user-perspective.

yep. a reason that i'm not too actively involved in typing issues at the moment. letting it play out for now.

So if this PR is prep for that then OK, but I would like to also see the impact on our current inline types and code changes to be kept to a minimum.

…structor (pandas-dev#46171)

burnpanck · 2022-08-16T11:06:26Z

As I noted in the linked issue #24983, I believe removing that type annotation is a bad idea. In particular, the fact that NaTType is not a subclass of Timestamp and/or Timedelta is not just an inconvenience for the static typing of user code, but also a risk; whenever a NaTType is being spit out, user code expecting either a Timedelta or Timestamp might break. Thus, the perceived inconvenience to the user is actually just the type checker doing its job: Alerting users from potential dangers through wrong assumptions. Sweeping that alert under the rug defeats the purpose of type annotations.

jbrockmendel · 2022-08-16T15:49:21Z

@mroeschke should we consider reverting for 1.5?

Dr-Irv · 2022-08-16T16:32:48Z

@mroeschke should we consider reverting for 1.5?

I wouldn't revert the whole PR. You could just revert the places where __new__() used to return Timestamp | NaT and now returns Timestamp due to this PR.

pandas-stubs always returns Timestamp because otherwise you'd always have to cast the result when using the constructor.

Having said that, it would be far better if NaT was subclassed in some way.

mroeschke · 2022-08-16T16:59:12Z

It would be good to have the more correct annotation back for 1.5.

I am not fully caught up what partial changes are needed to restore the old annotation, so if someone could provide a PR to correct that, it would be appreciated. Otherwise, if I am not able to get to it/figure it out I'll just fully revert for 1.5.

Dr-Irv · 2022-08-16T17:03:57Z

It would be good to have the more correct annotation back for 1.5.

I am not fully caught up what partial changes are needed to restore the old annotation, so if someone could provide a PR to correct that, it would be appreciated. Otherwise, if I am not able to get to it/figure it out I'll just fully revert for 1.5.

I will try to revert just that part.

Dr-Irv · 2022-08-16T17:46:50Z

A couple of notes:

The report here that the types were inaccurate was correct, but could not have been discovered without using development code, because the change to have typing not return NaTType was not released in pandas 1.4.x
Since doing this PR, we decided to have a separate pandas-stubs repo for public stubs, so the discussion above about type correctness for the public stubs belongs there (and will likely be hotly debated, if the debate ever starts)

I created a PR #48112 to revert just this typing change in the constructors, but if people are using pandas-stubs, the issue would still persist there.

Dr-Irv added 13 commits December 21, 2021 13:39

fix column_arrays for array manager

9e57d6d

merge with upstream/main

2abbc57

Merge remote-tracking branch 'upstream/main'

99158b1

Merge remote-tracking branch 'upstream/main'

0f4130d

Merge remote-tracking branch 'upstream/main'

24ecfe2

Merge remote-tracking branch 'upstream/main'

64f00d5

Merge remote-tracking branch 'upstream/main'

1ee6e00

Merge remote-tracking branch 'upstream/main'

5761bf2

remove dead code in arrays/interval.py

84b119f

Merge remote-tracking branch 'upstream/main'

5232375

Undo Revert "remove dead code in arrays/interval.py"

64feddd

This reverts commit 84b119f.

Merge remote-tracking branch 'upstream/main'

1cc352a

remove NaTType as possible result of Timestamp and Timedelta constructor

39b3b98

Dr-Irv requested review from jbrockmendel and twoertwein February 27, 2022 21:00

Dr-Irv added Timedelta Timedelta data type Timestamp pd.Timestamp and associated methods Typing type annotations, mypy/pyright type checking labels Feb 27, 2022

jreback reviewed Mar 2, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into nonattype

5abe369

jreback requested changes Mar 6, 2022

View reviewed changes

Dr-Irv added 2 commits March 7, 2022 11:30

Merge remote-tracking branch 'upstream/main' into nonattype

d980ebe

Add DatetimeNaTType type

e79d253

Change test for NaT in io/formats/format.py

jreback added this to the 1.5 milestone Mar 7, 2022

jreback approved these changes Mar 7, 2022

View reviewed changes

jreback merged commit 622c92d into pandas-dev:main Mar 8, 2022

simonjayhawkins reviewed Apr 6, 2022

View reviewed changes

yehoshuadimarsky pushed a commit to yehoshuadimarsky/pandas that referenced this pull request Jul 13, 2022

TYP: remove NaTType as possible result of Timestamp and Timedelta con…

29f026d

…structor (pandas-dev#46171)

burnpanck mentioned this pull request Aug 16, 2022

Separate NaT values for Timedelta ("NaTD") and Period? #24983

Open

mroeschke mentioned this pull request Aug 16, 2022

RLS: 1.5 #45223

Closed

Dr-Irv mentioned this pull request Aug 16, 2022

revert Timestamp and Timedelta constructors typing allowing NaTType return #48112

Merged

Dr-Irv deleted the nonattype branch February 13, 2023 20:56

randolf-scholz mentioned this pull request Apr 29, 2024

ENH: Introduce type-safe constructors for Timestamp and Timedelta. #58475

Open

3 tasks

TYP: remove NaTType as possible result of Timestamp and Timedelta constructor #46171

TYP: remove NaTType as possible result of Timestamp and Timedelta constructor #46171

Conversation

Dr-Irv commented Feb 27, 2022

jbrockmendel commented Feb 27, 2022

Dr-Irv commented Feb 27, 2022

twoertwein commented Feb 28, 2022 • edited Loading

Dr-Irv commented Feb 28, 2022

jbrockmendel commented Feb 28, 2022

Dr-Irv commented Feb 28, 2022

twoertwein commented Feb 28, 2022

Dr-Irv commented Feb 28, 2022

jbrockmendel commented Feb 28, 2022

Dr-Irv commented Feb 28, 2022

twoertwein commented Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback 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

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

jreback commented Mar 7, 2022

jbrockmendel commented Mar 8, 2022

jreback commented Mar 8, 2022

twoertwein commented Apr 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonjayhawkins commented Apr 6, 2022

simonjayhawkins commented Apr 6, 2022

simonjayhawkins commented Apr 6, 2022

twoertwein commented Apr 6, 2022

simonjayhawkins commented Apr 6, 2022

burnpanck commented Aug 16, 2022

jbrockmendel commented Aug 16, 2022

Dr-Irv commented Aug 16, 2022

mroeschke commented Aug 16, 2022

Dr-Irv commented Aug 16, 2022

Dr-Irv commented Aug 16, 2022

twoertwein commented Feb 28, 2022 •

edited

Loading

twoertwein commented Mar 1, 2022 •

edited

Loading