Skip to content

BUG: Period[us] start_time off by 1 nanosecond #31475

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

Merged
merged 9 commits into from
Feb 4, 2020
1 change: 1 addition & 0 deletions doc/source/whatsnew/v1.1.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ Datetimelike
- Bug in :class:`Timestamp` where constructing :class:`Timestamp` from ambiguous epoch time and calling constructor again changed :meth:`Timestamp.value` property (:issue:`24329`)
- :meth:`DatetimeArray.searchsorted`, :meth:`TimedeltaArray.searchsorted`, :meth:`PeriodArray.searchsorted` not recognizing non-pandas scalars and incorrectly raising ``ValueError`` instead of ``TypeError`` (:issue:`30950`)
- Bug in :class:`Timestamp` where constructing :class:`Timestamp` with dateutil timezone less than 128 nanoseconds before daylight saving time switch from winter to summer would result in nonexistent time (:issue:`31043`)
- Bug in :meth:`Period.to_timestamp`, :meth:`Period.start_time` with microsecond frequency returning a timestamp one nanosecond earlier than the correct time (:issue:`31475`)

Timedelta
^^^^^^^^^
Expand Down
9 changes: 8 additions & 1 deletion pandas/_libs/tslibs/period.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ PyDateTime_IMPORT
from pandas._libs.tslibs.np_datetime cimport (
npy_datetimestruct, dtstruct_to_dt64, dt64_to_dtstruct,
pandas_datetime_to_datetimestruct, check_dts_bounds,
NPY_DATETIMEUNIT, NPY_FR_D)
NPY_DATETIMEUNIT, NPY_FR_D, NPY_FR_us)

cdef extern from "src/datetime/np_datetime.h":
int64_t npy_datetimestruct_to_datetime(NPY_DATETIMEUNIT fr,
Expand Down Expand Up @@ -1186,6 +1186,13 @@ cdef int64_t period_ordinal_to_dt64(int64_t ordinal, int freq) except? -1:
if ordinal == NPY_NAT:
return NPY_NAT

if freq == 11000:
# Microsecond, avoid get_date_info to prevent floating point errors
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to fix get_date_info instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe. My first two tries didnt work.

Copy link
Contributor

Choose a reason for hiding this comment

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

so, is it possible to use this path for all freqs?

Copy link
Member Author

Choose a reason for hiding this comment

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

definitely not for the lower-frequency freqs because of their to_end attrs e.g. Q-DEC, Q-NOV.

pandas_datetime_to_datetimestruct(ordinal, NPY_FR_us, &dts)
Copy link
Contributor

Choose a reason for hiding this comment

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

if this path is kept, then I would do if/else here and have: L 1192, L1194 after (as they are repeated at L1197, 1198)

Copy link
Member Author

Choose a reason for hiding this comment

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

will do

Copy link
Contributor

Choose a reason for hiding this comment

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

k looks fine, what about higher freq than microsecond? these are ok in the get_date_info path?

Copy link
Member Author

Choose a reason for hiding this comment

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

what about higher freq than microsecond? these are ok in the get_date_info path?

I've got another branch im working on to handle that; it'll end up de-duplicating some code in the process

Copy link
Contributor

Choose a reason for hiding this comment

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

kk

check_dts_bounds(&dts)
# Equivalent: return ordinal * 1000
return dtstruct_to_dt64(&dts)

get_date_info(ordinal, freq, &dts)
check_dts_bounds(&dts)
return dtstruct_to_dt64(&dts)
Expand Down
19 changes: 18 additions & 1 deletion pandas/tests/scalar/period/test_asfreq.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from pandas._libs.tslibs.frequencies import INVALID_FREQ_ERR_MSG, _period_code_map
from pandas.errors import OutOfBoundsDatetime

from pandas import Period, offsets
from pandas import Period, Timestamp, offsets


class TestFreqConversion:
Expand Down Expand Up @@ -656,6 +656,23 @@ def test_conv_secondly(self):

assert ival_S.asfreq("S") == ival_S

def test_conv_microsecond(self):
# GH#31475 Avoid floating point errors dropping the start_time to
# before the beginning of the Period
per = Period("2020-01-30 15:57:27.576166", freq="U")
assert per.ordinal == 1580399847576166

start = per.start_time
expected = Timestamp("2020-01-30 15:57:27.576166")
assert start == expected
assert start.value == per.ordinal * 1000

per2 = Period("2300-01-01", "us")
with pytest.raises(OutOfBoundsDatetime, match="2300-01-01"):
per2.start_time
with pytest.raises(OutOfBoundsDatetime, match="2300-01-01"):
per2.end_time

def test_asfreq_mult(self):
# normal freq to mult freq
p = Period(freq="A", year=2007)
Expand Down