-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
#21128 DOC: Add documentation for freq='infer' option of DatetimeIndex and TimedeltaIndex constructors #21201
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
Conversation
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.
Thanks @kirakrishnan, looking good! A few comments:
doc/source/timedeltas.rst
Outdated
@@ -363,6 +363,12 @@ or ``np.timedelta64`` objects. Passing ``np.nan/pd.NaT/nat`` will represent miss | |||
pd.TimedeltaIndex(['1 days', '1 days, 00:00:05', | |||
np.timedelta64(2,'D'), datetime.timedelta(days=2,seconds=2)]) | |||
|
|||
In [5]: pd.TimedeltaIndex(['0 days', '10 days', '20 days']) |
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.
I think you can remove this first example, since the existing example above is similar. I'm open to leaving this in though, if you'd rather keep this as a second example.
doc/source/timedeltas.rst
Outdated
@@ -363,6 +363,12 @@ or ``np.timedelta64`` objects. Passing ``np.nan/pd.NaT/nat`` will represent miss | |||
pd.TimedeltaIndex(['1 days', '1 days, 00:00:05', | |||
np.timedelta64(2,'D'), datetime.timedelta(days=2,seconds=2)]) | |||
|
|||
In [5]: pd.TimedeltaIndex(['0 days', '10 days', '20 days']) | |||
Out[5]: TimedeltaIndex(['0 days', '10 days', '20 days'], dtype='timedelta64[ns]', freq=None) | |||
|
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.
Can you add a sentence here to highlight the freq='infer'
is being used here, and what it does, similar to the sentence added to the docstring.
doc/source/timedeltas.rst
Outdated
In [5]: pd.TimedeltaIndex(['0 days', '10 days', '20 days']) | ||
Out[5]: TimedeltaIndex(['0 days', '10 days', '20 days'], dtype='timedelta64[ns]', freq=None) | ||
|
||
In [4]: pd.TimedeltaIndex(['0 days', '10 days', '20 days'], freq='infer') |
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.
Can you put this in a .. ipython:: python
block? A new block will need to be created if you add the sentence I suggested above. This essentially allows sphinx to generate the python output when the docs are being built. You'll only need to add the input line (without In [4]:
) since the output will be generated. See the existing code sample a few lines above for an example of how to set this up.
doc/source/timeseries.rst
Outdated
@@ -185,6 +185,13 @@ options like ``dayfirst`` or ``format``, so use ``to_datetime`` if these are req | |||
|
|||
pd.Timestamp('2010/11/12') | |||
|
|||
You can also use the `DatetimeIndex` constructor directly: | |||
In [3]: pd.DatetimeIndex(['2018-01-01', '2018-01-03', '2018-01-05']) |
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.
Can you put this in a .. ipython:: python
block? (same procedure as in my previous comment)
doc/source/timeseries.rst
Outdated
You can also use the `DatetimeIndex` constructor directly: | ||
In [3]: pd.DatetimeIndex(['2018-01-01', '2018-01-03', '2018-01-05']) | ||
Out[3]: DatetimeIndex(['2018-01-01', '2018-01-03', '2018-01-05'], dtype='datetime64[ns]', freq=None) | ||
|
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.
Can you add a sentence here to highlight the freq='infer' is being used here and modify the example below in a .. ipython:: python
block? (similar sentence as described in my timedeltas.rst
comment)
pandas/core/indexes/datetimes.py
Outdated
@@ -186,7 +186,10 @@ class DatetimeIndex(DatelikeOps, TimelikeOps, DatetimeIndexOpsMixin, | |||
copy : bool | |||
Make a copy of input ndarray | |||
freq : string or pandas offset object, optional | |||
One of pandas date offset strings or corresponding objects | |||
One of pandas date offset strings or corresponding objects. The string | |||
'infer' can be passed in order to allow users to set the frequency of |
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.
I think this would be a little more direct if the "to allow users" is removed, so it just reads as "...in order to set the frequency...".
pandas/core/indexes/timedeltas.py
Outdated
freq: a frequency for the index, optional | ||
freq : string or pandas offset object, optional | ||
One of pandas date offset strings or corresponding objects. The string | ||
'infer' can be passed in order to allow users to set the frequency of |
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.
same regarding "to allow users"
to keep my local updated
Hi @jschendel , I made the changes you suggested and committed it. when I try to create pull request I am getting this message |
Codecov Report
@@ Coverage Diff @@
## master #21201 +/- ##
=======================================
Coverage 91.84% 91.84%
=======================================
Files 153 153
Lines 49505 49505
=======================================
Hits 45466 45466
Misses 4039 4039
Continue to review full report at Codecov.
|
lgtm. should have a comment about actually setting the e.g.
|
Yes, but I'll create a separate issue for that with some more specific details. Would be nice to update the |
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.
Looks good! A few very minor things but should be ready to merge afterwards.
@@ -363,6 +363,13 @@ or ``np.timedelta64`` objects. Passing ``np.nan/pd.NaT/nat`` will represent miss | |||
pd.TimedeltaIndex(['1 days', '1 days, 00:00:05', | |||
np.timedelta64(2,'D'), datetime.timedelta(days=2,seconds=2)]) | |||
|
|||
'infer' can be passed in order to set the frequency of the index as the inferred frequency | |||
upon creation |
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.
Can you add a colon after creation, i.e. "...upon creation:"
@@ -185,6 +185,17 @@ options like ``dayfirst`` or ``format``, so use ``to_datetime`` if these are req | |||
|
|||
pd.Timestamp('2010/11/12') | |||
|
|||
You can also use the `DatetimeIndex` constructor directly: | |||
|
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.
Can you put the first DatetimeIndex
example here (the one without freq='infer'
)?
You can also use the `DatetimeIndex` constructor directly: | ||
|
||
'infer' can be passed in order to set the frequency of the index as the inferred frequency | ||
upon creation |
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.
Can you add a colon after creation, i.e. "...upon creation:"
freq : string or pandas offset object, optional | ||
One of pandas date offset strings or corresponding objects. The string | ||
'infer' can be passed in order to set the frequency of the index as the | ||
inferred frequency upon creation |
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.
Looks like this line has one extra space at the beginning. The "i" in inferred should be aligned with the single quote on the line above. Not sure if this impacts how the output will be rendered though.
@kirakrishnan if you can update |
@jschendel if you wouldn't mind updating, this looks ok otherwise. (pls merge on green as well). |
@jreback : it doesn't look like I have push access to update this (unless I'm missing something?), so will close and reboot as a new PR in a little bit. |
Closing in favor of #21566 |
git diff upstream/master -u -- "*.py" | flake8 --diff