-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
FIX BUG: Timestamp __add__/__sub__ DateOffset with nanoseconds lost. #43968
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
Changes from 22 commits
2208507
ee19a4d
3e38ab2
5718e4f
e1ae149
326790b
83453da
ae4b133
a43030d
22cadbf
1c89c03
a2ad274
9bc6e97
c9ddf14
4780316
1fecfa6
e92c1a9
006c073
ab064b4
8e42c64
5ca3472
37d19b5
5acad37
9cda54a
0ee9a6f
860a62b
4600a64
8cf5746
a210baf
98646cd
e899fbe
906f546
24fe768
209ba9e
f56f52b
3f49876
b7297ef
5cbae96
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -657,14 +657,6 @@ def test_rule_code(self): | |
assert alias == (_get_offset(alias) * 5).rule_code | ||
|
||
|
||
def test_dateoffset_misc(): | ||
oset = offsets.DateOffset(months=2, days=4) | ||
# it works | ||
oset.freqstr | ||
|
||
assert not offsets.DateOffset(months=2) == 2 | ||
|
||
|
||
def test_freq_offsets(): | ||
off = BDay(1, offset=timedelta(0, 1800)) | ||
assert off.freqstr == "B+30Min" | ||
|
@@ -780,6 +772,41 @@ def test_tick_normalize_raises(tick_classes): | |
cls(n=3, normalize=True) | ||
|
||
|
||
@pytest.mark.parametrize( | ||
"cases", | ||
[ | ||
("nanoseconds", 1, Timestamp("1970-01-01 00:00:00.000000001")), | ||
("nanoseconds", 5, Timestamp("1970-01-01 00:00:00.000000005")), | ||
("nanoseconds", -1, Timestamp("1969-12-31 23:59:59.999999999")), | ||
("microseconds", 1, Timestamp("1970-01-01 00:00:00.000001")), | ||
("microseconds", -1, Timestamp("1969-12-31 23:59:59.999999")), | ||
("seconds", 1, Timestamp("1970-01-01 00:00:01")), | ||
("seconds", -1, Timestamp("1969-12-31 23:59:59")), | ||
("minutes", 1, Timestamp("1970-01-01 00:01:00")), | ||
("minutes", -1, Timestamp("1969-12-31 23:59:00")), | ||
("hours", 1, Timestamp("1970-01-01 01:00:00")), | ||
("hours", -1, Timestamp("1969-12-31 23:00:00")), | ||
("days", 1, Timestamp("1970-01-02 00:00:00")), | ||
("days", -1, Timestamp("1969-12-31 00:00:00")), | ||
("weeks", 1, Timestamp("1970-01-08 00:00:00")), | ||
("weeks", -1, Timestamp("1969-12-25 00:00:00")), | ||
("months", 1, Timestamp("1970-02-01 00:00:00")), | ||
("months", -1, Timestamp("1969-12-01 00:00:00")), | ||
("years", 1, Timestamp("1971-01-01 00:00:00")), | ||
("years", -1, Timestamp("1969-01-01 00:00:00")), | ||
], | ||
) | ||
def test_dateoffset_add_sub(cases): | ||
time_unit, num, expected = cases | ||
offset = DateOffset(**{time_unit: num}) | ||
ts = Timestamp(0) + offset | ||
jbrockmendel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
assert ts == expected | ||
ts -= offset | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jbrockmendel For the question "do we have any test cases with both nano !=0 and hasattr(self, "nanoseconds")?" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. The case I think is uncovered is where both have a nanosecond and the DateOffset has a relativedelta component:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks and I think the PR can pass this test and let me add a new test case for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test failed, maybe I need rebase my local branch in docker from master to see why. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In DateOffset._apply we call _as_datetime which drops nanos, so we need to either a) not do that or b) add them back in at some point There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The building doc process failed maybe because it ran the command with flag and there is a warning: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I ran the checks again, 2 of them failed because of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rebased from master branch, the errors listed as below:
@jbrockmendel It seems that all the errors above are not related with my PR, could you take a look? |
||
assert ts == Timestamp(0) | ||
ts = offset + Timestamp(0) | ||
assert ts == expected | ||
|
||
|
||
@pytest.mark.parametrize( | ||
"attribute", | ||
[ | ||
|
@@ -795,3 +822,11 @@ def test_dateoffset_immutable(attribute): | |
msg = "DateOffset objects are immutable" | ||
with pytest.raises(AttributeError, match=msg): | ||
setattr(offset, attribute, 5) | ||
|
||
|
||
def test_dateoffset_misc(): | ||
oset = offsets.DateOffset(months=2, days=4) | ||
# it works | ||
oset.freqstr | ||
|
||
assert not offsets.DateOffset(months=2) == 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have any test cases with both nano !=0 and hasattr(self, "nanoseconds")?
if you can make this comment (L189) more specific that'd be a nice bonus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we have. It's in
pandas/tests/tseries/offsets/test_offsets.py
L816, let me mention you in that line.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also added the explanation for L189.