CLN: remove pydt_to_i8 #30854
CLN: remove pydt_to_i8 #30854
Conversation
pandas/_libs/tslibs/offsets.pyx
Outdated
| @@ -233,7 +233,7 @@ def _to_dt64(dt, dtype='datetime64'): | |||
| # numpy.datetime64('2013-05-01T02:00:00.000000+0200') | |||
| # Thus astype is needed to cast datetime to datetime64[D] | |||
| if getattr(dt, 'tzinfo', None) is not None: | |||
| i8 = pydt_to_i8(dt) | |||
| i8 = dt.timestamp() * 10**9 | |||
There was a problem hiding this comment.
Is there any chance of losing precision doing it this way? I think can be somewhere around the 2 ** 60 range for floats
There was a problem hiding this comment.
I don't think so, pydt obects only have 6 digits after the decimal, so we should be OK. Besides, later in this function we case to datetime64[D]
|
@jbrockmendel can you merge master? |
|
rebased+green |
pandas/_libs/tslibs/offsets.pyx
Outdated
| @@ -233,7 +233,7 @@ def _to_dt64D(dt): | |||
| # numpy.datetime64('2013-05-01T02:00:00.000000+0200') | |||
| # Thus astype is needed to cast datetime to datetime64[D] | |||
| if getattr(dt, 'tzinfo', None) is not None: | |||
| i8 = pydt_to_i8(dt) | |||
| i8 = np.int64(dt.timestamp() * 10**9) | |||
There was a problem hiding this comment.
Hmm I'm still a little hung up on this. Just to illustrate:
dt1 = datetime.datetime(2020, 1, 17, 16, 1, 9, 136686)
>>> dt2 = pd.Timestamp(np.int64(dt1.timestamp() * 10**9))
>>> dt2.nanosecond
80I suppose not totally equivalent given the later cast to datetime[D] but this does lose some precision right? If so could lead to some hard to find bugs, but could also misunderstand
There was a problem hiding this comment.
I think you're right to be hung up on this, but for a different reason. I had forgotten that the stdlib datetime.timestamp behaves different from pd.Timestamp.timestamp for tz-naive datetimes. I'll give this another look.
There was a problem hiding this comment.
i take it back, i was right the first time. this usage is only in the tzaware case, so we're all good
There was a problem hiding this comment.
shouldn't do what convet_datetime_to_tsobject does? this doesn't appear to have the same timezone behavior
There was a problem hiding this comment.
this doesn't appear to have the same timezone behavior
On the previous line we are checking that we are only doing this in the tzaware case, in which datetime.timestamp and pd.Timestamp.timestamp behave the same.
There was a problem hiding this comment.
as i said i think you should re-use convert_datetime_to_tsobject rather than doing this.
There was a problem hiding this comment.
We can do this, but it gets appreciably more verbose because we need special handling for datetime vs Timestamp. The version in the PR ATM is correct for both datetime and Timestmap.
pandas/_libs/tslibs/offsets.pyx
Outdated
| @@ -233,7 +233,7 @@ def _to_dt64D(dt): | |||
| # numpy.datetime64('2013-05-01T02:00:00.000000+0200') | |||
| # Thus astype is needed to cast datetime to datetime64[D] | |||
| if getattr(dt, 'tzinfo', None) is not None: | |||
| i8 = pydt_to_i8(dt) | |||
| i8 = np.int64(dt.timestamp() * 10**9) | |||
There was a problem hiding this comment.
shouldn't do what convet_datetime_to_tsobject does? this doesn't appear to have the same timezone behavior
|
thanks |
It's only used in one non-test place, and all supported python versions now support datetime.timestamp, so we can use that directly. Its a little bit less performant, but not used often.