Skip to content

#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

Closed
wants to merge 10 commits into from

Conversation

kirakrishnan
Copy link

@kirakrishnan kirakrishnan commented May 25, 2018

Copy link
Member

@jschendel jschendel left a 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:

@@ -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'])
Copy link
Member

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.

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

Copy link
Member

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.

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')
Copy link
Member

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.

@@ -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'])
Copy link
Member

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)

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)

Copy link
Member

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)

@@ -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
Copy link
Member

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

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
Copy link
Member

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"

@kirakrishnan
Copy link
Author

Hi @jschendel , I made the changes you suggested and committed it. when I try to create pull request I am getting this message Able to merge. These branches can be automatically merged. . Please tell me if I can leave it or Do I need to do something else to merge the changes?

@codecov
Copy link

codecov bot commented May 26, 2018

Codecov Report

Merging #21201 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #21201   +/-   ##
=======================================
  Coverage   91.84%   91.84%           
=======================================
  Files         153      153           
  Lines       49505    49505           
=======================================
  Hits        45466    45466           
  Misses       4039     4039
Flag Coverage Δ
#multiple 90.23% <ø> (ø) ⬆️
#single 41.88% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/indexes/timedeltas.py 91.24% <ø> (ø) ⬆️
pandas/core/indexes/datetimes.py 95.78% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c2844a...e1a80eb. Read the comment docs.

@jreback jreback added Docs Datetime Datetime data dtype Timedelta Timedelta data type Frequency DateOffsets labels May 29, 2018
@jreback jreback added this to the 0.23.1 milestone May 29, 2018
@jreback
Copy link
Contributor

jreback commented May 29, 2018

lgtm.

should have a comment about actually setting the freq attribute as well?

e.g.

dti.freq = 'D' or whatever

@jschendel
Copy link
Member

should have a comment about actually setting the freq attribute as well?

Yes, but I'll create a separate issue for that with some more specific details. Would be nice to update the freq docstring in addition to timeseries.rst and timedeltas.rst.

Copy link
Member

@jschendel jschendel left a 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
Copy link
Member

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:

Copy link
Member

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
Copy link
Member

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
Copy link
Member

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.

@jreback jreback removed this from the 0.23.1 milestone Jun 4, 2018
@jreback
Copy link
Contributor

jreback commented Jun 4, 2018

@kirakrishnan if you can update

@jreback
Copy link
Contributor

jreback commented Jun 19, 2018

@jschendel if you wouldn't mind updating, this looks ok otherwise. (pls merge on green as well).

@jreback jreback added this to the 0.24.0 milestone Jun 19, 2018
@jschendel
Copy link
Member

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

@jschendel
Copy link
Member

Closing in favor of #21566

@jschendel jschendel closed this Jun 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Datetime Datetime data dtype Docs Frequency DateOffsets Timedelta Timedelta data type
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DOC: Add documentation for freq='infer' option of DatetimeIndex and TimedeltaIndex constructors
3 participants