Skip to content

BUG: pd.DateOffset handle milliseconds #50020

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 23 commits into from
Dec 22, 2022
Merged

BUG: pd.DateOffset handle milliseconds #50020

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
48d9dfa
BUG: fix milliseconds and add test
markopacak Dec 1, 2022
65cd5da
BUG: move condition-check outside main if-block
markopacak Dec 2, 2022
f989674
DOC: update whatsnew
markopacak Dec 3, 2022
b3a5ef1
DOC: update whatsnew
markopacak Dec 3, 2022
37427dd
Merge branch 'main' into bug-dateoffset-milliseconds
markopacak Dec 4, 2022
705d2d6
CLN: reduce docstring
markopacak Dec 4, 2022
c1be7c6
CLN: remove return statement
markopacak Dec 4, 2022
1788491
Merge branch 'main' into bug-dateoffset-milliseconds
markopacak Dec 4, 2022
ae0b529
Merge branch 'pandas-dev:main' into bug-dateoffset-milliseconds
markopacak Dec 8, 2022
64c9ee3
TST: update assertions
markopacak Dec 8, 2022
65015c8
CLN: rework _determine_offset and add tests
markopacak Dec 8, 2022
ef7f118
CLN: def to cdef
markopacak Dec 8, 2022
b2b7473
BUG: GH49897 raise for invalid arguments + test
markopacak Dec 15, 2022
c2cf081
TST: GH 49897 remove useless test
markopacak Dec 19, 2022
3981b04
TST: BUG49897 convert millis to micros so relativedelta can use them
markopacak Dec 19, 2022
d80e780
Merge branch 'main' into bug-dateoffset-milliseconds
markopacak Dec 20, 2022
27109a7
TST: GH49897 test millis + micros
markopacak Dec 22, 2022
76fafe2
Merge remote-tracking branch 'upstream/main' into bug-dateoffset-mill…
markopacak Dec 22, 2022
2ad8c60
CLN: GH49897 remove comment
markopacak Dec 22, 2022
2f913a8
Merge branch 'bug-dateoffset-milliseconds' of github.com:markopacak/p…
markopacak Dec 22, 2022
8eb83d0
DOC: GH49897 update rst
markopacak Dec 22, 2022
309e296
CLN: re-order if-conditions
markopacak Dec 22, 2022
0d85a74
TST: GH49897 test raise
markopacak Dec 22, 2022
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
1 change: 1 addition & 0 deletions doc/source/whatsnew/v2.0.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,6 +659,7 @@ Datetimelike
- Bug in ``pandas.tseries.holiday.Holiday`` where a half-open date interval causes inconsistent return types from :meth:`USFederalHolidayCalendar.holidays` (:issue:`49075`)
- Bug in rendering :class:`DatetimeIndex` and :class:`Series` and :class:`DataFrame` with timezone-aware dtypes with ``dateutil`` or ``zoneinfo`` timezones near daylight-savings transitions (:issue:`49684`)
- Bug in :func:`to_datetime` was raising ``ValueError`` when parsing :class:`Timestamp`, ``datetime``, or ``np.datetime64`` objects with non-ISO8601 ``format`` (:issue:`49298`, :issue:`50036`)
- Bug in :class:`DateOffset` when constructing with milliseconds in combination with another argument (:issue:`49897`)
Copy link
Member

Choose a reason for hiding this comment

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

can we be more specific? like Bug in :class:DateOffset` was throwing ``TypeError```..., or whatever it was

-

Timedelta
Expand Down
5 changes: 3 additions & 2 deletions pandas/_libs/tslibs/offsets.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,9 @@ cdef _determine_offset(kwds):

use_relativedelta = False
if len(kwds_no_nanos) > 0:
if any(k in _kwds_use_relativedelta for k in kwds_no_nanos):
if "milliseconds" in kwds_no_nanos:
offset = timedelta(**kwds_no_nanos)
Copy link
Member

Choose a reason for hiding this comment

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

I think this would still fail if a user passed DateOffset(milliseconds=1, month=1) i.e. any non-timedelta keyword constructor was passed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it fails. What should be done in cases like this? Both timedelta and relativedelta will throw an error.

TBH I don't know how to fix this bug without trying to rework the entire function. Does it make sense to broaden up the scope of this issue?

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be okay to broaden the scope if all the existing tests pass. Generally I would imagine there should be checking that kwds (besides nanoseconds) should either all be valid for timedelta or relativetimedelta

Copy link
Contributor Author

@markopacak markopacak Dec 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is what I had in mind.

All tests in tests/arithmetic/test_datetime64.py and tests/tseries/offsets/test_offsets.py pass.

Added one parametrized test.

DateOffset(milliseconds=1, month=1) fails because I don't know what should happen in this case (see last return stmt in first link)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I just open another PR with the rework linked above and close this one?

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be fine to just update this PR with your proposed refactor

Copy link
Member

Choose a reason for hiding this comment

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

could we do something like

if 'milliseconds' in kwds:
     if "microseconds in kwds:
           raise ...
     kwds['microseconds'] = kwds.pop('milliseconds') * 1000

(and something similar in the if "millisecond" in kwds_no_nanos case below)

Copy link
Contributor Author

@markopacak markopacak Dec 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I added this code instead of the exception block (full code here) :

if "millisecond" in kwds_no_nanos:
    micro = kwds_no_nanos.pop("millisecond") * 1000
    kwds_no_nanos["microsecond"] = kwds_no_nanos.get("microsecond", 0) + micro

if "milliseconds" in kwds_no_nanos:
    micro = kwds_no_nanos.pop("milliseconds") * 1000
    kwds_no_nanos["microseconds"] = kwds_no_nanos.get("microseconds", 0) + micro

but it fails 2 tests in pandas/tests/tseries/offsets/test_offsets.py, both of which are meant to intentionally fail if we pass millisecond. So we aren't breaking any real functionality, I guess.

TestDateOffset.test_constructor[millisecond] 
[XPASS(strict)] Constructing DateOffset object with `millisecond` is not yet supported.

test_valid_relativedelta_kwargs[millisecond] 
[XPASS(strict)] Constructing DateOffset object with `millisecond` is not yet supported.

However, I think the requirement is to have all the existing tests pass.

There is still the case where we receive minutes (arg valid for timedelta) + another arg valid for relativedelta only.
In general, the last return needs to be discussed: What do we do in "all other cases"?

elif any(k in _kwds_use_relativedelta for k in kwds_no_nanos):
if "millisecond" in kwds_no_nanos:
raise NotImplementedError(
"Using DateOffset to replace `millisecond` component in "
Expand Down Expand Up @@ -1163,7 +1165,6 @@ cdef class RelativeDeltaOffset(BaseOffset):

def __init__(self, n=1, normalize=False, **kwds):
BaseOffset.__init__(self, n, normalize)

off, use_rd = _determine_offset(kwds)
object.__setattr__(self, "_offset", off)
object.__setattr__(self, "_use_relativedelta", use_rd)
Expand Down
4 changes: 4 additions & 0 deletions pandas/tests/tseries/offsets/test_offsets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,6 +739,10 @@ def test_eq(self):

assert DateOffset(milliseconds=3) != DateOffset(milliseconds=7)

def test_milliseconds_combination(self):
# 10525
DateOffset(days=1, milliseconds=1)
Copy link
Member

Choose a reason for hiding this comment

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

Can you add an assertion that the object has been properly constructed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed



class TestOffsetNames:
def test_get_offset_name(self):
Expand Down