BUG: pd.DateOffset handle milliseconds #50020

markopacak · 2022-12-02T18:06:15Z

closes BUG: pd.DateOffset only handles milliseconds keyword argument if other keyword args are shorter than day/days #49897
Tests added and passed if fixing a bug or adding a new feature
All code checks passed.
Added type annotations to new arguments/methods/functions.
Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

markopacak · 2022-12-03T12:51:44Z

IMO after this fix, _determine_offset needs some rework. The function has been continuously patched over-time and has become unreadable.

I'm testing some implementations in a separate branch

WillAyd

thanks for the PR. looks like a merge conflict

pandas/tests/tseries/offsets/test_offsets.py

WillAyd · 2022-12-03T19:59:44Z

pandas/_libs/tslibs/offsets.pyx

@@ -314,6 +314,9 @@ cdef _determine_offset(kwds):

    use_relativedelta = False
    if len(kwds_no_nanos) > 0:
+        if "milliseconds" in kwds_no_nanos:
+            return timedelta(**kwds_no_nanos), use_relativedelta


Can you assign to offset and use_relativedelta and just let the function return naturally? I think flows better with the other branching

WillAyd · 2022-12-05T23:04:26Z

pandas/tests/tseries/offsets/test_offsets.py

@@ -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)


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

WillAyd

Lgtm. @jbrockmendel or @MarcoGorelli care to look?

mroeschke · 2022-12-08T18:46:10Z

pandas/_libs/tslibs/offsets.pyx

@@ -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)


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

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?

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

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)

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

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

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)

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"?

mroeschke · 2022-12-13T19:31:01Z

pandas/_libs/tslibs/offsets.pyx

+        # This also handles "milliseconds" (plur): see GH 49897
+        return timedelta(**kwds_no_nanos), False
+
+    # TODO what to do in this case?


I would imagine this should raise since there are mixed relativedelta and timedelta based kwds

id have to double-check but i think pytimedelta keywords are also valid relativedelta keywords

All except milliseconds.
And for some reason, minutes is not present in _kwds_use_relativedelta, despite being a valid relativedelta arg.
It breaks tests if I add it

With our current configuration, we end up in this last return stmt, when:

~~minutes + another relativedelta arg~~

~~milliseconds + another relativedelta arg~~

any invalid arg for either relativedelta or timedelta

I think it makes sense to raise, e.g: Invalid keyword configuration.

Edit: fixed the above

markopacak · 2022-12-19T10:14:06Z

I made it:

milliseconds now work in combination with sub-/above-daily args (see tests)
code is cleaned
no existing tests are broken

It was tough to maintain compatibility with older tests. If new issues about this or related modules come up, feel free to tag me.

MarcoGorelli · 2022-12-19T22:13:55Z

thanks for working on this, I'll take a look soon

looks like CI didn't run for some reason, could you resolve the merge conflicts and push please? that should restart it

MarcoGorelli · 2022-12-20T15:53:08Z

pandas/tests/tseries/offsets/test_offsets.py

+    @pytest.mark.parametrize(
+        "offset_kwargs, expected_arg",
+        [
+            ({"seconds": 1, "milliseconds": 1}, "2022-01-01 00:00:01.001"),


should there be a test to check when both milliseconds and microseconds were passed, seeing as you sum them together?

(CI failures are unrelated BTW, if you fetch and merge upstream/main they should be fixed)

MarcoGorelli · 2022-12-20T16:15:00Z

pandas/_libs/tslibs/offsets.pyx

+        "seconds", "microseconds", "milliseconds", "minutes", "hours",
+    }
+
+    # check timedelta before relativedelta: maintain compatibility with old tests


"maintain compatibility with old tests" is usually a given, not sure it needs a comment 😄

MarcoGorelli · 2022-12-20T16:16:02Z

doc/source/whatsnew/v2.0.0.rst

@@ -794,6 +794,7 @@ Datetimelike
 - Bug in :func:`to_datetime` was raising ``ValueError`` when parsing empty string and non-ISO8601 format was passed. Now, empty strings will be parsed as :class:`NaT`, for compatibility with how is done for ISO8601 formats (:issue:`50251`)
 - Bug in :class:`Timestamp` was showing ``UserWarning``, which was not actionable by users, when parsing non-ISO8601 delimited date strings (:issue:`50232`)
 - Bug in :func:`to_datetime` was showing misleading ``ValueError`` when parsing dates with format containing ISO week directive and ISO weekday directive (:issue:`50308`)
+- Bug in :class:`DateOffset` when constructing with milliseconds in combination with another argument (:issue:`49897`)


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

…iseconds # Conflicts: # doc/source/whatsnew/v2.0.0.rst

…andas into bug-dateoffset-milliseconds # Conflicts: # doc/source/whatsnew/v2.0.0.rst

MarcoGorelli

Minor comment, but the rest looks good!

doc/source/whatsnew/v2.0.0.rst

MarcoGorelli

Nice work!

Sorry, just noticed a couple more things, but other than that, really solid work here, nice 💪 Much simpler to understand like this

MarcoGorelli · 2022-12-22T10:46:35Z

pandas/_libs/tslibs/offsets.pyx

+    raise ValueError(
+        f"Invalid argument/s or bad combination of arguments: {list(kwds.keys())}"
+    )



is there a test that hits this?

Actually, ever since minutes was added to kwds_use_relativedelta, and milliseconds is converted to microseconds, the only case where you raise is for invalid args

So it would be something like:

def test_offset_invalid_arguments(self): with pytest.raises(ValueError, match="^Invalid argument/s or bad combination of arguments"): DateOffset(picoseconds=1) # or any other invalid arg

Does it make sense?

yeah looks good - if we're adding code, let's make sure it's covered

MarcoGorelli · 2022-12-22T10:48:39Z

pandas/_libs/tslibs/offsets.pyx

+        raise NotImplementedError(
+            "Using DateOffset to replace `millisecond` component in "
+            "datetime object is not supported. Use "
+            "`microsecond=timestamp.microsecond % 1000 + ms * 1000` "
+            "instead."
+        )


granted, there wasn't a test which hit this to begin with, but there should probably be one? could we add one please (either here or in a separate PR)

Found these 2, not sure if they qualify as valid test

pandas/pandas/tests/tseries/offsets/test_offsets.py

Lines 930 to 944 in 8eb83d0

@pytest.mark.parametrize("kwd", sorted(liboffsets._relativedelta_kwds))

def test_valid_relativedelta_kwargs(kwd, request):

if kwd == "millisecond":

request.node.add_marker(

pytest.mark.xfail(

raises=NotImplementedError,

reason="Constructing DateOffset object with `millisecond` is not "

"yet supported.",

)

)

# Check that all the arguments specified in liboffsets._relativedelta_kwds

# are in fact valid relativedelta keyword args

DateOffset(**{kwd: 1})

pandas/pandas/tests/tseries/offsets/test_offsets.py

Lines 616 to 629 in 8eb83d0

@pytest.mark.parametrize("kwd", sorted(liboffsets._relativedelta_kwds))

def test_constructor(self, kwd, request):

if kwd == "millisecond":

request.node.add_marker(

pytest.mark.xfail(

raises=NotImplementedError,

reason="Constructing DateOffset object with `millisecond` is not "

"yet supported.",

)

)

offset = DateOffset(**{kwd: 2})

assert offset.kwds == {kwd: 2}

assert getattr(offset, kwd) == 2

ah, thanks! all good then

MarcoGorelli · 2022-12-22T10:50:27Z

pandas/_libs/tslibs/offsets.pyx


+    kwds_no_nanos = {k: v for k, v in kwds.items() if k not in nanos}
+
+    if "millisecond" in kwds_no_nanos:


this could happen earlier, right? and be simplified to if "millisecond" in kwds?

MarcoGorelli

Really nice, thanks @markopacak

mroeschke · 2022-12-22T22:49:23Z

Thanks @markopacak

markopacak added 2 commits December 1, 2022 16:52

BUG: fix milliseconds and add test

48d9dfa

BUG: move condition-check outside main if-block

65cd5da

markopacak marked this pull request as ready for review December 3, 2022 12:49

markopacak added 2 commits December 3, 2022 14:09

DOC: update whatsnew

f989674

DOC: update whatsnew

b3a5ef1

WillAyd requested changes Dec 3, 2022

View reviewed changes

markopacak and others added 4 commits December 4, 2022 16:25

Merge branch 'main' into bug-dateoffset-milliseconds

37427dd

CLN: reduce docstring

705d2d6

CLN: remove return statement

c1be7c6

Merge branch 'main' into bug-dateoffset-milliseconds

1788491

WillAyd requested changes Dec 5, 2022

View reviewed changes

markopacak and others added 2 commits December 8, 2022 11:45

Merge branch 'pandas-dev:main' into bug-dateoffset-milliseconds

ae0b529

TST: update assertions

64c9ee3

mroeschke added the Frequency DateOffsets label Dec 8, 2022

WillAyd approved these changes Dec 8, 2022

View reviewed changes

mroeschke reviewed Dec 8, 2022

View reviewed changes

MarcoGorelli self-requested a review December 8, 2022 19:03

markopacak added 2 commits December 8, 2022 22:02

CLN: rework _determine_offset and add tests

65015c8

CLN: def to cdef

ef7f118

mroeschke reviewed Dec 13, 2022

View reviewed changes

markopacak added 3 commits December 15, 2022 12:37

BUG: GH49897 raise for invalid arguments + test

b2b7473

TST: GH 49897 remove useless test

c2cf081

TST: BUG49897 convert millis to micros so relativedelta can use them

3981b04

Merge branch 'main' into bug-dateoffset-milliseconds

d80e780

MarcoGorelli suggested changes Dec 20, 2022

View reviewed changes

markopacak added 2 commits December 22, 2022 08:26

TST: GH49897 test millis + micros

27109a7

Merge remote-tracking branch 'upstream/main' into bug-dateoffset-mill…

76fafe2

…iseconds # Conflicts: # doc/source/whatsnew/v2.0.0.rst

markopacak added 2 commits December 22, 2022 09:10

CLN: GH49897 remove comment

2ad8c60

Merge branch 'bug-dateoffset-milliseconds' of github.com:markopacak/p…

2f913a8

…andas into bug-dateoffset-milliseconds # Conflicts: # doc/source/whatsnew/v2.0.0.rst

MarcoGorelli reviewed Dec 22, 2022

View reviewed changes

doc/source/whatsnew/v2.0.0.rst Outdated Show resolved Hide resolved

DOC: GH49897 update rst

8eb83d0

MarcoGorelli suggested changes Dec 22, 2022

View reviewed changes

markopacak added 2 commits December 22, 2022 17:12

CLN: re-order if-conditions

309e296

TST: GH49897 test raise

0d85a74

MarcoGorelli approved these changes Dec 22, 2022

View reviewed changes

mroeschke approved these changes Dec 22, 2022

View reviewed changes

mroeschke merged commit 7bc699f into pandas-dev:main Dec 22, 2022

markopacak deleted the bug-dateoffset-milliseconds branch January 22, 2023 19:58


	@pytest.mark.parametrize("kwd", sorted(liboffsets._relativedelta_kwds))
	def test_valid_relativedelta_kwargs(kwd, request):
	if kwd == "millisecond":
	request.node.add_marker(
	pytest.mark.xfail(
	raises=NotImplementedError,
	reason="Constructing DateOffset object with `millisecond` is not "
	"yet supported.",
	)
	)
	# Check that all the arguments specified in liboffsets._relativedelta_kwds
	# are in fact valid relativedelta keyword args
	DateOffset(**{kwd: 1})

	@pytest.mark.parametrize("kwd", sorted(liboffsets._relativedelta_kwds))
	def test_constructor(self, kwd, request):
	if kwd == "millisecond":
	request.node.add_marker(
	pytest.mark.xfail(
	raises=NotImplementedError,
	reason="Constructing DateOffset object with `millisecond` is not "
	"yet supported.",
	)
	)
	offset = DateOffset(**{kwd: 2})
	assert offset.kwds == {kwd: 2}
	assert getattr(offset, kwd) == 2


		kwds_no_nanos = {k: v for k, v in kwds.items() if k not in nanos}

		if "millisecond" in kwds_no_nanos:

Uh oh!

BUG: pd.DateOffset handle milliseconds #50020

BUG: pd.DateOffset handle milliseconds #50020

Uh oh!

Conversation

markopacak commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markopacak commented Dec 3, 2022

Uh oh!

WillAyd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillAyd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markopacak Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markopacak Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markopacak Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markopacak commented Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarcoGorelli commented Dec 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoGorelli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MarcoGorelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

markopacak commented Dec 2, 2022 •

edited

Loading

markopacak Dec 8, 2022 •

edited

Loading

markopacak Dec 13, 2022 •

edited

Loading

markopacak Dec 15, 2022 •

edited

Loading

markopacak commented Dec 19, 2022 •

edited

Loading