Skip to content

CLN: remove pydt_to_i8 #30854

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jan 24, 2020
Merged

CLN: remove pydt_to_i8 #30854

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b84def2
CLN: remove pydt_to_i8
jbrockmendel Jan 9, 2020
e2db91d
Merge branch 'master' of https://github.com/pandas-dev/pandas into cl…
jbrockmendel Jan 10, 2020
b2a135a
explicitly cast to int64
jbrockmendel Jan 10, 2020
84f38c5
Merge branch 'master' of https://github.com/pandas-dev/pandas into cl…
jbrockmendel Jan 16, 2020
57a311b
Merge branch 'master' of https://github.com/pandas-dev/pandas into cl…
jbrockmendel Jan 16, 2020
68bf4ae
Merge branch 'master' of https://github.com/pandas-dev/pandas into cl…
jbrockmendel Jan 17, 2020
fa99d6e
Merge branch 'master' of https://github.com/pandas-dev/pandas into cl…
jbrockmendel Jan 19, 2020
b7128fc
use convert_datetime_to_tsobject
jbrockmendel Jan 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
explicitly cast to int64
  • Loading branch information
@jbrockmendel
jbrockmendel committed Jan 10, 2020
commit b2a135a8cc11d86151834a66c33441086222a81f
2 changes: 1 addition & 1 deletion pandas/_libs/tslibs/offsets.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = dt.timestamp() * 10**9
i8 = np.int64(dt.timestamp() * 10**9)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
80

I 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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i take it back, i was right the first time. this usage is only in the tzaware case, so we're all good

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't do what convet_datetime_to_tsobject does? this doesn't appear to have the same timezone behavior

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as i said i think you should re-use convert_datetime_to_tsobject rather than doing this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

dt = tz_convert_single(i8, UTC, dt.tzinfo)
dt = np.int64(dt).astype('datetime64[ns]')
else:
Expand Down