-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: Timestamp(pd.NA) raising TypeError #54102
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
Conversation
pandas/_libs/tslibs/timestamps.pyx
Outdated
elif checknull_with_nat_and_na(ts_input): | ||
return NaT |
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 the
if ts.value == NPY_NAT:
return NaT
be removed from below now?
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.
It doesn't look like it can be removed. Just tried it and this starts to raise if its removed:
pd.Timestamp(np.datetime64("NaT"))
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.
ah right, thanks, checknull checks for pandas NaT, not numpy NaT
does it work to do the check a bit deeper? i.e.
diff --git a/pandas/_libs/tslibs/conversion.pyx b/pandas/_libs/tslibs/conversion.pyx
index 57b7754b08..4d494d2282 100644
--- a/pandas/_libs/tslibs/conversion.pyx
+++ b/pandas/_libs/tslibs/conversion.pyx
@@ -1,4 +1,5 @@
import numpy as np
+from pandas._libs.missing cimport checknull_with_nat_and_na
cimport numpy as cnp
from libc.math cimport log10
@@ -268,7 +269,7 @@ cdef _TSObject convert_to_tsobject(object ts, tzinfo tz, str unit,
if isinstance(ts, str):
return convert_str_to_tsobject(ts, tz, unit, dayfirst, yearfirst)
- if ts is None or ts is NaT:
+ if checknull_with_nat_and_na(ts):
obj.value = NPY_NAT
elif is_datetime64_object(ts):
reso = get_supported_reso(get_datetime64_unit(ts))
diff --git a/pandas/_libs/tslibs/timestamps.pyx b/pandas/_libs/tslibs/timestamps.pyx
index 80b7c2d2f5..59640a3ef4 100644
--- a/pandas/_libs/tslibs/timestamps.pyx
+++ b/pandas/_libs/tslibs/timestamps.pyx
@@ -1882,9 +1882,6 @@ class Timestamp(_Timestamp):
hour or 0, minute or 0, second or 0, fold=fold or 0)
unit = None
- elif checknull_with_nat_and_na(ts_input):
- return NaT
-
if getattr(ts_input, "tzinfo", None) is not None and tz is not None:
raise ValueError("Cannot pass a datetime or Timestamp with tzinfo with "
"the tz parameter. Use tz_convert instead.")
?
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 - updated. thanks
Is it obvious we want to allow this? If we consider pd.NA as meaningfully distinct from pd.NaT, then silently converting would be wrong |
I would've thought so - the docs say
and |
The meaning of "missing" has changed since that doc was written. In particular NaT has nan-like semantics. |
I dont have a strong opinion but I'll note:
|
fair enough. no objection here. |
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.
cool, going to go ahead and approve then, thanks @lukemanley !
This might cause issue if we every introduced a nullable numpy datetime/timedelta types with |
Does the numpy-nullable part matter here? timestamp[pyarrow] already behaves that way |
Sure, I was just confirming that there wasn't appetite to have BaseMaskedArray support datetime/timedelta types for example |
Thanks @lukemanley |
doc/source/whatsnew/v2.1.0.rst
file if fixing a bug or adding a new feature.