DOC: Correct inconsistent description on default DateOffset setting #36516

GYHHAHA · 2020-09-21T04:20:50Z

correct inconsistent doc description on default DateOffset setting

closes DOC: default value for pd.DateOffset() is 24 hours instead of 1 calendar day #36512

correct inconsistent doc description on default DateOffset setting

arw2019

thanks @GYHHAHA

doc/source/user_guide/timeseries.rst

arw2019 · 2020-09-21T04:43:21Z

Also heads up there are linting issues

##[error]doc/source/user_guide/timeseries.rst:834:83:E501:line too long (105 > 79 characters)

to be resolved before merge

remove the example for DateOffset default setting

GYHHAHA · 2020-09-21T05:27:25Z

why fail and how can I fix?

MarcoGorelli · 2020-09-21T07:31:26Z

What's wrong with "one calendar day"?

GYHHAHA · 2020-09-21T07:36:26Z

>>>ts = pd.Timestamp('2011-3-27 00:00:00', tz='Europe/Helsinki')

One calendar day will get the result below.

>>>ts + pd.DateOffset(days=1)
Timestamp('2011-03-28 00:00:00+0300', tz='Europe/Helsinki')

But now the default action is add absolute 24 hours.

>>>ts + pd.DateOffset()
Timestamp('2011-03-28 01:00:00+0300', tz='Europe/Helsinki')
>>>ts + pd.DateOffset(hours=24)
Timestamp('2011-03-28 01:00:00+0300', tz='Europe/Helsinki')

MarcoGorelli · 2020-09-21T07:42:04Z

Ah, thanks!

GYHHAHA · 2020-09-21T07:44:21Z

So how can I fix these failings? Seems that I only change one line.

MarcoGorelli · 2020-09-21T07:47:10Z

They're unrelated, you'll probably need to merge upstream/master later

jreback · 2020-09-21T10:56:08Z

doc/source/user_guide/timeseries.rst

@@ -846,7 +846,7 @@ into ``freq`` keyword arguments. The available date offsets and associated frequ
    :header: "Date Offset", "Frequency String", "Description"
    :widths: 15, 15, 65

-    :class:`~pandas.tseries.offsets.DateOffset`, None, "Generic offset class, defaults to 1 calendar day"
+    :class:`~pandas.tseries.offsets.DateOffset`, None, "Generic offset class, defaults to 24 hours"


cc @mroeschke

GYHHAHA · 2020-09-21T10:56:53Z

But still a little strange about the default value, why not just set it a calendar day since the offset objects are specially designed for calendar duration rules?

mroeschke · 2020-09-21T17:20:33Z

I think this should ideally default to calendar day instead 24 hours as well. This would require a deprecation cycle related to #7418 that would deprecate all arguments that don't end with s.

Would you be interested in making that change instead @GYHHAHA?

GYHHAHA · 2020-09-22T00:28:57Z

I will take a try.

Change the default setting for DateOffset to a calendar day.

add a test for dateoffset default value

pep8speaks · 2020-09-22T01:30:41Z

Hello @GYHHAHA! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file pandas/tests/tslibs/test_liboffsets.py:

Line 172:1: W293 blank line contains whitespace

Comment last updated at 2020-09-22 06:42:18 UTC

change doc to original description

resolve PEP 8 issues

jreback · 2020-09-22T01:43:41Z

pandas/_libs/tslibs/offsets.pyx

@@ -319,7 +319,8 @@ cdef _determine_offset(kwds):
            # sub-daily offset - use timedelta (tz-aware)
            offset = timedelta(**kwds_no_nanos)
    else:
-        offset = timedelta(1)
+        offset = relativedelta(**{'days':1})


@mroeschke I don't know how we can easily deprecate this, nor can we simply change this.

Can't simply change this in the next release, but I think we could deprecate the keyword arguments and slate for removal in 2.0.

GYHHAHA · 2020-09-22T02:37:36Z

For #7418, can we just convert any argument in kwds_no_nanos that end with s to the one with s? @jreback

GYHHAHA · 2020-09-22T07:03:54Z

Also since the _determine_offset doesn't handle the nanosecond or nanoseconds, thus here is a bug related to this.

>>>ts = pd.Timestamp('2020-1-1')
>>>ts + pd.DateOffset(nanoseconds=1000)
Timestamp('2020-01-02 00:00:00')

Caused by the default offset = timedelta(1) setting. @mroeschke

GYHHAHA · 2020-09-23T00:46:15Z

Ok, I will keep this in original form, and should I continue to change the "1 calendar day" to "24 hours" in the DOC?

Maybe a warning need to be added when use singular arguments.
I find in DOC: hour/hours (and other plurals) based on dateutil.relativedelta #7418 ts.replace(pd.DateOffset(...)) is suggested, is this a good way to make these replacement actions?
The above-mentioned nanoseconds BUG needs a fix, but since this relates to the default setting, will it be troublesome to handle?

Thanks. @jreback @mroeschke

mroeschke · 2020-09-23T16:35:31Z

Yes I think for this PR, you can just make the documentation change.

You can follow up with another PR to deprecate the DateOffset arguments that do not end in s if interested.

Also if you could submit a bug report with the nanosecond example, that'd be great.

GYHHAHA · 2020-09-24T03:31:40Z

Here are also some problems.

DOC says "However, all DateOffset subclasses that are an hour or smaller (Hour, Minute, Second, Milli, Micro, Nano) behave like Timedelta and respect absolute time.".

But though DateOffset(days=1) indeed use a calendar day, still offsets.Day(1) use 24 absolute hours.

Is it necessary to keep this consistency?

Besides, in #36592 I suggest to add replace method for time-related replacement, but now I also think it may not be a good choice.

However, we can only use ts + pd.DateOffset(day=5) to change the absolute year/day/hour/... use the singular form argument which will be deprecated (if we make this deprecation), thus how can we do this absolute time replacement by other offsets object? (like changing year to 2000 or day to 22 or month to 5)

Thanks! @mroeschke

mroeschke · 2020-09-24T03:38:47Z

See #22864 about the offsets.Day consistency.

For absolute time changes, I think users should just stick to using Timedelta objects. DateOffset object should only operate with relative time changes.

GYHHAHA · 2020-09-24T04:01:46Z

agree with jbrockmendel

If we're talking reasonably long-term, I think we should get rid of the Tick classes entirely and use Timedelta in their place

It will be much more clear to only consider the relative time (day, week, month ,year, ...) for offsets and tick time (nano, ..., minute, hour, absolute day) for timedelta.

github-actions · 2020-10-27T00:17:03Z

This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.

jreback · 2020-10-31T19:20:01Z

@mroeschke this lgtu?

mroeschke · 2020-10-31T23:00:29Z

Thanks @GYHHAHA

…andas-dev#36516) * Update timeseries.rst correct inconsistent doc description on default DateOffset setting * Update timeseries.rst remove the example for DateOffset default setting * Update offsets.pyx Change the default setting for DateOffset to a calendar day. * Update test_liboffsets.py add a test for dateoffset default value * Update timeseries.rst change doc to original description * Update test_liboffsets.py resolve PEP 8 issues * Update timeseries.rst * Update test_liboffsets.py * Update offsets.pyx * Change default Offset setting description in DOC * Update timeseries.rst * Update timeseries.rst

Update timeseries.rst

ea8a773

correct inconsistent doc description on default DateOffset setting

arw2019 approved these changes Sep 21, 2020

View reviewed changes

doc/source/user_guide/timeseries.rst Outdated Show resolved Hide resolved

Update timeseries.rst

8a07c3a

remove the example for DateOffset default setting

jreback requested changes Sep 21, 2020

View reviewed changes

GYHHAHA added 2 commits September 22, 2020 09:21

Update offsets.pyx

90d4f8a

Change the default setting for DateOffset to a calendar day.

Update test_liboffsets.py

eeb89c6

add a test for dateoffset default value

GYHHAHA added 2 commits September 22, 2020 09:32

Update timeseries.rst

0d5e1a3

change doc to original description

Update test_liboffsets.py

f245260

resolve PEP 8 issues

GYHHAHA changed the title ~~Update timeseries.rst~~ Change default setting for DateOffset to a calendar day Sep 22, 2020

jreback added the Frequency DateOffsets label Sep 22, 2020

jreback requested changes Sep 22, 2020

View reviewed changes

Update timeseries.rst

2399eb8

GYHHAHA added 3 commits September 24, 2020 09:21

Update test_liboffsets.py

bc0a02e

Update offsets.pyx

a8b3189

Change default Offset setting description in DOC

dca455a

GYHHAHA changed the title ~~Change default setting for DateOffset to a calendar day~~ DOC: Correct inconsistent description on default DateOffset setting Sep 24, 2020

GYHHAHA mentioned this pull request Sep 24, 2020

ENH: replace method for more time-related type #36592

Open

Update timeseries.rst

ee41b7b

GYHHAHA mentioned this pull request Sep 26, 2020

DEPR: DateOffset args don't end in 's' #36660

Closed

github-actions bot added the Stale label Oct 27, 2020

Update timeseries.rst

0defdc6

jreback added this to the 1.2 milestone Oct 31, 2020

jreback added Docs and removed Stale labels Oct 31, 2020

mroeschke approved these changes Oct 31, 2020

View reviewed changes

mroeschke merged commit 10de71c into pandas-dev:master Oct 31, 2020

GYHHAHA deleted the patch-1 branch October 31, 2020 23:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOC: Correct inconsistent description on default DateOffset setting #36516

DOC: Correct inconsistent description on default DateOffset setting #36516

GYHHAHA commented Sep 21, 2020 •

edited

Loading

arw2019 left a comment

arw2019 commented Sep 21, 2020

GYHHAHA commented Sep 21, 2020

MarcoGorelli commented Sep 21, 2020

GYHHAHA commented Sep 21, 2020

MarcoGorelli commented Sep 21, 2020

GYHHAHA commented Sep 21, 2020

MarcoGorelli commented Sep 21, 2020

jreback Sep 21, 2020

GYHHAHA commented Sep 21, 2020

mroeschke commented Sep 21, 2020

GYHHAHA commented Sep 22, 2020

pep8speaks commented Sep 22, 2020 •

edited

Loading

jreback Sep 22, 2020

mroeschke Sep 22, 2020

GYHHAHA commented Sep 22, 2020 •

edited

Loading

GYHHAHA commented Sep 22, 2020 •

edited

Loading

GYHHAHA commented Sep 23, 2020 •

edited

Loading

mroeschke commented Sep 23, 2020

GYHHAHA commented Sep 24, 2020

mroeschke commented Sep 24, 2020

GYHHAHA commented Sep 24, 2020 •

edited

Loading

github-actions bot commented Oct 27, 2020

jreback commented Oct 31, 2020

mroeschke commented Oct 31, 2020

DOC: Correct inconsistent description on default DateOffset setting #36516

DOC: Correct inconsistent description on default DateOffset setting #36516

Conversation

GYHHAHA commented Sep 21, 2020 • edited Loading

arw2019 left a comment

Choose a reason for hiding this comment

arw2019 commented Sep 21, 2020

GYHHAHA commented Sep 21, 2020

MarcoGorelli commented Sep 21, 2020

GYHHAHA commented Sep 21, 2020

MarcoGorelli commented Sep 21, 2020

GYHHAHA commented Sep 21, 2020

MarcoGorelli commented Sep 21, 2020

jreback Sep 21, 2020

Choose a reason for hiding this comment

GYHHAHA commented Sep 21, 2020

mroeschke commented Sep 21, 2020

GYHHAHA commented Sep 22, 2020

pep8speaks commented Sep 22, 2020 • edited Loading

Comment last updated at 2020-09-22 06:42:18 UTC

jreback Sep 22, 2020

Choose a reason for hiding this comment

mroeschke Sep 22, 2020

Choose a reason for hiding this comment

GYHHAHA commented Sep 22, 2020 • edited Loading

GYHHAHA commented Sep 22, 2020 • edited Loading

GYHHAHA commented Sep 23, 2020 • edited Loading

mroeschke commented Sep 23, 2020

GYHHAHA commented Sep 24, 2020

mroeschke commented Sep 24, 2020

GYHHAHA commented Sep 24, 2020 • edited Loading

github-actions bot commented Oct 27, 2020

jreback commented Oct 31, 2020

mroeschke commented Oct 31, 2020

GYHHAHA commented Sep 21, 2020 •

edited

Loading

pep8speaks commented Sep 22, 2020 •

edited

Loading

GYHHAHA commented Sep 22, 2020 •

edited

Loading

GYHHAHA commented Sep 22, 2020 •

edited

Loading

GYHHAHA commented Sep 23, 2020 •

edited

Loading

GYHHAHA commented Sep 24, 2020 •

edited

Loading