FIX BUG: Timestamp add/sub DateOffset with nanoseconds lost. #43968

tushushu · 2021-10-11T13:23:29Z

closes BUG: DateOffset with nanoseconds does not apply to Timestamp #43892
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

tushushu · 2021-10-11T13:30:41Z

Hey guys, not only Timestamp.__add__(DateOffset), but also Timestamp.__sub__(DateOffset) not works. Should I fix the __sub__ method in this PR or should I open another PR? Thanks~

jbrockmendel · 2021-10-11T14:41:07Z

Should I fix the sub method in this PR or should I open another PR

Doing it in the same PR is fine

jbrockmendel · 2021-10-11T14:42:09Z

pandas/_libs/tslibs/offsets.pyx

@@ -83,6 +83,10 @@ from .timestamps import Timestamp
 # ---------------------------------------------------------------------
 # Misc Helpers

+cdef bint is_dateoffset_object(object obj):


can you add a docstring, specifically make it clear when to use this vs is_offset_object

Sure, that makes sense.

the comment you have here is not useful, add a doc-string not dependent on a particular patch.

Maybe this function is not that necessary. It could be enough by using is_offset_object(obj) and hasattr(obj, "nanoseconds") to determine the need to deal with "nanoseconds".

When calling _TimeStamp.__add__(self, other) with other is an Offset object, it can not meet any condition of the if-else chain. And it will call _TimeStamp.__add__(self, other._offset) instead. I cannot find where this logic is implemented, but I guess we could explicitly check the Offset type in the if-else chain.

jreback

this needs tests

jreback · 2021-10-16T14:42:26Z

pandas/_libs/tslibs/offsets.pyx

@@ -83,6 +83,10 @@ from .timestamps import Timestamp
 # ---------------------------------------------------------------------
 # Misc Helpers

+cdef bint is_dateoffset_object(object obj):


the comment you have here is not useful, add a doc-string not dependent on a particular patch.

pandas/_libs/tslibs/timestamps.pyx

tushushu · 2021-10-17T06:28:26Z

this needs tests

I am going to put some tests later.

tushushu · 2021-10-17T10:42:27Z

Found another bug: the DateOffset(nanoseconds=1) object is not constructed correctly.

from pandas.tseries.offsets import DateOffset

print(DateOffset(nanoseconds=1)._offset.__repr__())

# datetime.timedelta(1)

Which should be "datetime.timedelta(0)".

pandas/_libs/tslibs/timestamps.pyx

jbrockmendel · 2021-10-18T18:41:39Z

pandas/_libs/tslibs/timestamps.pyx


        if is_any_td_scalar(other):
-            nanos = delta_to_nanoseconds(other)
+            nanos += delta_to_nanoseconds(other)


are there any cases that go through the hasattr(other, "nanoseconds") and also through here?

If a case go through the hasattr(other, "nanoseconds"), it must go through here. For example, Timestampe(0) + DateOffset(nanoseconds=1) will go through both branches. Because line 289 other = other._offset converts other to a td_scalar.

it must go through here [...] Because line 289 other = other._offset converts other to a td_scalar.

Doesn't it sometimes convert to a relativedelta object (which isnt a td_scalar)?

@jbrockmendel According to the codes below, we can see self._offset is a relativedelta object only if there is no nanosecond / nanoseconds keyword parameter.

kwds_no_nanos = dict( (k, v) for k, v in kwds.items() if k not in ('nanosecond', 'nanoseconds') ) # TODO: Are nanosecond and nanoseconds allowed somewhere? _kwds_use_relativedelta = ('years', 'months', 'weeks', 'days', 'year', 'month', 'week', 'day', 'weekday', 'hour', 'minute', 'second', 'microsecond') use_relativedelta = False if len(kwds_no_nanos) > 0: if any(k in _kwds_use_relativedelta for k in kwds_no_nanos): offset = relativedelta(**kwds_no_nanos) use_relativedelta = True

pandas/_libs/tslibs/offsets.pyx

jbrockmendel · 2021-10-19T16:34:45Z

pandas/_libs/tslibs/timestamps.pyx

@@ -282,9 +282,17 @@ cdef class _Timestamp(ABCTimestamp):
    def __add__(self, other):
        cdef:
            int64_t nanos = 0
+        # In order to correctly handle DateOffset object with nanoseconds.


nitpick: can you put this inside the if is_offset_object and make clear it is a RelativeDelta offset, not any DateOffset

These lines have been moved to RelativeDelta.applymethod.

jbrockmendel · 2021-10-19T16:38:22Z

pandas/_libs/tslibs/timestamps.pyx

+                other = other._offset
+            else:
+                nanos -= other.nanoseconds
+                other = -other._offset


so in principle Timestamp.__add__ shouldn't need to know anything about the RelativeDeltaOffset implementation. Could we change RelativeDeltaOffset.apply to handle nanoseconds correctly instead?

OK, let me read the RelativeDeltaOffset implementation to see how should I move the logic away from here.

Now the RelativeDeltaOffset.apply can handle nanoseconds correctly.

jbrockmendel · 2021-10-19T16:40:25Z

pandas/tests/tseries/offsets/test_offsets.py

+)
+def test_dateoffset_add_sub(cases):
+    time_unit, expected = cases
+    offset = DateOffset(**{time_unit: 1})


if im reading RelativeDeltaOffset.apply right, i think this might be off with n!=1?

Let me read the RelativeDeltaOffset.apply codes first, and make more test cases with n!=1 here.

I changed the codes, aslo add test case with n == 5, and it works.

pandas/tests/tseries/offsets/test_offsets.py

jreback · 2021-12-03T16:29:59Z

@jbrockmendel over to you, merge when ready

tushushu · 2021-12-04T14:52:45Z

pandas/tests/tseries/offsets/test_offsets.py

+        ({"months": -1}, 0, "1969-12-01 00:00:00"),
+        ({"years": 1}, 0, "1971-01-01 00:00:00"),
+        ({"years": -1}, 0, "1969-01-01 00:00:00"),
+        ({"minutes": 2, "nanoseconds": 9}, 4, "1970-01-01 00:02:00.000000013"),


@jbrockmendel Here is the test case you suggest. The test can be passed in my local machine, but the travis is too slow, will look at the result later.

nitpick: id make a separate test for this specific case (could have useful name/comment) and avoid the extra clutter in the parametrize here

That makes sense, just done.

jreback · 2021-12-05T22:39:12Z

hmm tests seems to be failing, can you merge master. these don't seem related, but it is possible something changed.

jbrockmendel · 2021-12-05T22:43:47Z

yah i think merging master should fix most/all of the CI complaints

tushushu · 2021-12-07T17:43:27Z

After merging the master PR to current, the previous tests passed, but got a new one

'Hypothesis test_where_inplace_casting(data=[None, None, -4399835, -5869]) produces unreliable results: 
Falsified on the first call but did not on a subsequent one

…pandas into fix-bug-dateoffset-nanoseconds

jreback · 2021-12-07T23:21:51Z

hmm lots of things are failing

…pandas into fix-bug-dateoffset-nanoseconds

tushushu · 2021-12-08T12:34:02Z

I accidentally deleted a colon -_-

jreback · 2021-12-08T13:50:25Z

hmm something must have changed recently as https://github.com/pandas-dev/pandas/runs/4457135516?check_suite_focus=true are failing and look related

jreback · 2021-12-08T13:53:44Z

actually @tushushu these are failing elsewhere so nothing to do rn. will have you rebase once this is ironed out.

jbrockmendel · 2021-12-13T17:15:29Z

@tushushu the py310 failures were also unrelated, have now been fixed. one more rebase pls.

tushushu · 2021-12-17T11:20:35Z

@tushushu the py310 failures were also unrelated, have now been fixed. one more rebase pls.

Let me have a try~

jreback · 2021-12-17T22:41:39Z

thanks @tushushu for sticking with this!

tushushu · 2021-12-18T07:47:51Z

Thanks @jreback and @jbrockmendel for reviewing this PR.

fix Timestamp + Dateoffset bugs.

2208507

jbrockmendel reviewed Oct 11, 2021

View reviewed changes

tushushu changed the title ~~FIX BUG: Timestamp adds DateOffset with nanoseconds lost.~~ FIX BUG: Timestamp __add__/__sub__ DateOffset with nanoseconds lost. Oct 16, 2021

tushushu added 3 commits October 16, 2021 12:24

Add doc string and fix UT.

ee19a4d

fix UT

3e38ab2

Trim Trailing Whitespace

5718e4f

jreback requested changes Oct 16, 2021

View reviewed changes

jreback added the Frequency DateOffsets label Oct 16, 2021

tushushu added 5 commits October 17, 2021 10:45

remove is_dateoffset_obj func

e1ae149

explicitly check the Offset type

326790b

fix bugs.

83453da

fix bugs

ae4b133

try to fix __sub__ method.

a43030d

fix flake8 check.

22cadbf

tushushu added 4 commits October 17, 2021 18:49

add tests for dateoffset add method.

1c89c03

fix offset init.

a2ad274

fix black code style check.

9bc6e97

test sub method.

c9ddf14

jbrockmendel reviewed Oct 18, 2021

View reviewed changes

pandas/_libs/tslibs/timestamps.pyx Outdated Show resolved Hide resolved

jbrockmendel reviewed Oct 18, 2021

View reviewed changes

pandas/_libs/tslibs/offsets.pyx Show resolved Hide resolved

add an comment.

4780316

jbrockmendel reviewed Oct 19, 2021

View reviewed changes

pandas/tests/tseries/offsets/test_offsets.py Outdated Show resolved Hide resolved

makes the comment more detailed.

4600a64

test case as jbrockmendel demand

8cf5746

tushushu commented Dec 4, 2021

View reviewed changes

tushushu requested a review from jbrockmendel December 4, 2021 14:53

tushushu and others added 2 commits December 5, 2021 12:02

trigger the CI without a commit

a210baf

Merge branch 'pandas-dev:master' into fix-bug-dateoffset-nanoseconds

98646cd

tushushu added 2 commits December 6, 2021 15:54

Merge branch 'pandas-dev:master' into fix-bug-dateoffset-nanoseconds

e899fbe

Merge branch 'pandas-dev:master' into fix-bug-dateoffset-nanoseconds

906f546

tushushu and others added 3 commits December 8, 2021 01:51

split dateoffset test func

24fe768

Merge branch 'fix-bug-dateoffset-nanoseconds' of github.com:tushushu/…

209ba9e

…pandas into fix-bug-dateoffset-nanoseconds

Merge branch 'pandas-dev:master' into fix-bug-dateoffset-nanoseconds

f56f52b

tushushu added 2 commits December 8, 2021 20:32

fix typo

3f49876

Merge branch 'fix-bug-dateoffset-nanoseconds' of github.com:tushushu/…

b7297ef

…pandas into fix-bug-dateoffset-nanoseconds

Merge branch 'pandas-dev:master' into fix-bug-dateoffset-nanoseconds

5cbae96

jreback merged commit cd5a124 into pandas-dev:master Dec 17, 2021

jbrockmendel mentioned this pull request Dec 21, 2021

BUG: nanoseconds argument for DateOffset results in wrong output #36589

Closed

simonjayhawkins mentioned this pull request Feb 10, 2022

BUG: pandas.tseries.offsets.DateOffset behavior changes #45890

Closed

3 tasks

simonjayhawkins mentioned this pull request May 5, 2022

BUG: pd.date_range not factoring in nanoseconds in freq #46877

Closed

3 tasks

FIX BUG: Timestamp __add__/__sub__ DateOffset with nanoseconds lost. #43968

FIX BUG: Timestamp __add__/__sub__ DateOffset with nanoseconds lost. #43968

Conversation

tushushu commented Oct 11, 2021 • edited Loading

tushushu commented Oct 11, 2021

jbrockmendel commented Oct 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tushushu Oct 17, 2021 • edited Loading

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tushushu commented Oct 17, 2021

tushushu commented Oct 17, 2021 • edited Loading

Choose a reason for hiding this comment

tushushu Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tushushu Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Dec 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Dec 5, 2021

jbrockmendel commented Dec 5, 2021

tushushu commented Dec 7, 2021 • edited Loading

jreback commented Dec 7, 2021

tushushu commented Dec 8, 2021

jreback commented Dec 8, 2021

jreback commented Dec 8, 2021

jbrockmendel commented Dec 13, 2021

tushushu commented Dec 17, 2021

jreback commented Dec 17, 2021

tushushu commented Dec 18, 2021

FIX BUG: Timestamp add/sub DateOffset with nanoseconds lost. #43968

FIX BUG: Timestamp add/sub DateOffset with nanoseconds lost. #43968

tushushu commented Oct 11, 2021 •

edited

Loading

tushushu Oct 17, 2021 •

edited

Loading

tushushu commented Oct 17, 2021 •

edited

Loading

tushushu Oct 19, 2021 •

edited

Loading

tushushu Oct 20, 2021 •

edited

Loading

tushushu commented Dec 7, 2021 •

edited

Loading