DOC: Fixed timedelta docstring and examples by ryankarlos · Pull Request #23259 · pandas-dev/pandas

ryankarlos · 2018-10-20T21:43:56Z

closes Datetime units not listed in pd.to_timedelta docstring #23248
tests added / passed
passes git diff upstream/master --name-only -- "*.py" | grep "pandas/" | xargs flake8
passes python scripts/validate_docstrings.py pandas.to_timedelta

pep8speaks · 2018-10-20T21:43:58Z

Hello @ryankarlos! Thanks for updating the PR.

There are no PEP8 issues in the file pandas/core/tools/timedeltas.py !

Comment last updated on October 27, 2018 at 13:50 Hours UTC

ryankarlos · 2018-10-20T21:48:15Z

I've added in a few examples to illustrate 'M' and 'Y' outputs along with generating new columns in a dataframe using timedelta. Also corrected some linting issues and added a better summary. Any feedback would be welcome.

ryankarlos · 2018-10-20T22:35:21Z

Travis failing in feather --- ive seen other PRs having the same problem - not sure why it is ?

pandas/core/tools/timedeltas.py

mroeschke · 2018-10-21T00:36:59Z

pandas/core/tools/timedeltas.py

    TimedeltaIndex(['0 days', '1 days', '2 days', '3 days', '4 days'],
                   dtype='timedelta64[ns]', freq=None)

+    For `M` and `Y` units, `1M = 30D` and 1Y = 365D:


So there's an issue to deprecate 'M' and 'Y' for Timedelta, so I don't think it's a good idea to advertise these argument that are slated for deprecation.

#16344

@mroeschke sorry wasn't aware about the issue - ill remove it. Im thinking giving example for all the other units would be overkill - maybe a couple more ?
Maybe a reference to DateOffset in the description or examples would help ?

If you're up for it, you could address #16344 first and then add a note here saying "Day frequencies and higher are only supported.

Otherwise, I think this docstring has a good number of already examples, but demonstrating box would be a nice to have.

Just sent an initial PR for #16344 , still have work to do as its my first time fixing deprecation issues before.
What is the protocol for displaying deprecation warnings within docstrings - does it go under summary section or within a separate Notes section ? I guess it would make more sense to have a line under 'units' stating M an Y are not supported and then reference to pd.DateOffset or similar for M and Y ?

You can mention the deprecated parameters in the Notes section

mroeschke · 2018-10-21T00:37:29Z

pandas/core/tools/timedeltas.py

-    Convert argument to timedelta
+    Convert argument to timedelta.
+
+    Timedeltas are differences in times, expressed in difference units


Can you mention they are absolute differences in time.

mroeschke · 2018-10-21T17:03:27Z

pandas/core/tools/timedeltas.py

+    >>> pd.to_timedelta(np.arange(5), box=False)
+    array([0, 1, 2, 3, 4], dtype='timedelta64[ns]')
+
+    Add new column of dates from existing dates in a `DataFrame`


I'd remove this example as well; it demonstrates assignment more than to_timedelta

Do you mean the box example or the one below it ?

The one below it demonstrating DataFrame column assignment

codecov · 2018-10-25T06:32:15Z

Codecov Report

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

@@           Coverage Diff           @@
##           master   #23259   +/-   ##
=======================================
  Coverage   92.28%   92.28%           
=======================================
  Files         161      161           
  Lines       51451    51451           
=======================================
  Hits        47483    47483           
  Misses       3968     3968

Flag	Coverage Δ
#multiple	`90.68% <ø> (ø)`	⬆️
#single	`42.31% <ø> (+0.03%)`	⬆️

Impacted Files	Coverage Δ
pandas/core/tools/timedeltas.py	`98.03% <ø> (ø)`	⬆️

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 deb7b4d...a67649a. Read the comment docs.

mroeschke · 2018-10-25T15:47:43Z

cc @datapythonista if you could have a look.

ryankarlos · 2018-10-25T20:13:00Z

@mroeschke I haven't added the notes section for deprecated units yet - i was working concurrently on #23264 so wasn't sure if i should add that in before finishing that

datapythonista

Thanks for the work on this. Added some comments about the formatting.

datapythonista · 2018-10-25T23:14:17Z

pandas/core/tools/timedeltas.py

-        - If True returns a Timedelta/TimedeltaIndex of the results
-        - if False returns a np.timedelta64 or ndarray of values of dtype
-          timedelta64[ns]
+    arg : String, timedelta, list, tuple, 1-d array, or Series


str instead of String

datapythonista · 2018-10-25T23:16:25Z

pandas/core/tools/timedeltas.py

-          timedelta64[ns]
+    arg : String, timedelta, list, tuple, 1-d array, or Series
+          The argument which needs to be converted to timedelta.
+    unit : Integer or float, default ns


int instead of Integer. But doesn't make sense, seemsl like it should be a string.

Quotes around 'ns'

Oh Sorry yes - my bad - thanks for spotting this.

datapythonista · 2018-10-25T23:16:51Z

pandas/core/tools/timedeltas.py

+          The argument which needs to be converted to timedelta.
+    unit : Integer or float, default ns
+          Denotes the unit (D,h,m,s,ms,us,ns) of the arg.
+    box : Boolean, default True


bool instead of Boolean

datapythonista · 2018-10-25T23:17:39Z

pandas/core/tools/timedeltas.py

+          Denotes the unit (D,h,m,s,ms,us,ns) of the arg.
+    box : Boolean, default True
+          If True returns a Timedelta/TimedeltaIndex of the results.
+          if False returns a np.timedelta64 or ndarray of values of dtype


Use a list, it'll render better. Ignore the validation errors.

Thanks, thats why i removed them below to get rid of the validation errors - will include them.

datapythonista · 2018-10-25T23:18:02Z

pandas/core/tools/timedeltas.py

-        - If 'ignore', then invalid parsing will return the input
+          If 'raise', then invalid parsing will raise an exception.
+          If 'coerce', then invalid parsing will be set as NaT.
+          If 'ignore', then invalid parsing will return the input.


Keep the list.

datapythonista · 2018-10-25T23:18:37Z

pandas/core/tools/timedeltas.py

+    >>> pd.to_timedelta(np.arange(5), box=False)
+    array([0, 1, 2, 3, 4], dtype='timedelta64[ns]')
+
    See also


datapythonista · 2018-10-25T23:19:29Z

pandas/core/tools/timedeltas.py

+
    See also
    --------
    pandas.DataFrame.astype : Cast argument to a specified dtype.


remove the pandas. prefix

pandas/core/tools/timedeltas.py

ryankarlos · 2018-10-26T23:28:56Z

@datapythonista Thanks for the review. Added changes. Also rendered the docstring below:

datapythonista

looking good, added a few more comments.

datapythonista · 2018-10-27T03:30:56Z

pandas/core/tools/timedeltas.py

+    Convert argument to timedelta.
+
+    Timedeltas are absolute differences in times, expressed in difference
+    units e.g. days, hours, minutes, seconds. This method converts an argument


I think it'd look better to have the e.g. in brackets.

datapythonista · 2018-10-27T03:31:24Z

pandas/core/tools/timedeltas.py

+    arg : str, timedelta, list, tuple, 1-d array, or Series
+          The argument which needs to be converted to timedelta.
+    unit : str, default 'ns'
+          Denotes the unit (D,h,m,s,ms,us,ns) of the arg.


I'd add spaces after the commas.

datapythonista · 2018-10-27T03:34:22Z

pandas/core/tools/timedeltas.py


    Returns
    -------
    ret : timedelta64/arrays of timedelta64 if parsing succeeded


you can get rid of the ret, and leave here just the types timedelta64 or numpy.array of timedelta64. We should be able to automatically parse the line with the types, so no description should go there. A description on what is being returned, and clarification on when every type is being returned should go in the next line (indented).

datapythonista · 2018-10-27T03:36:11Z

pandas/core/tools/timedeltas.py

+          Denotes the unit (D,h,m,s,ms,us,ns) of the arg.
+    box : bool, default True
+        - If True returns a Timedelta/TimedeltaIndex of the results.
        - if False returns a np.timedelta64 or ndarray of values of dtype


Capital I

ryankarlos · 2018-10-27T10:36:29Z

@datapythonista Thanks, added an update. Im working concurrently on a related PR #23264 -- not sure if i should add the notes section on deprecated units here or in that PR ?

datapythonista · 2018-10-27T10:37:15Z

pandas/core/tools/timedeltas.py

    Parameters
    ----------
-    arg : str, timedelta, list, tuple, 1-d array, or Series
+    arg : str,  timedelta,  list,  tuple,  1-d array,  or Series


the missing spaces were down, in the (D,h,m,s...), which should be (D, h, m, s,...).

Just leave a space after the commas here.

datapythonista

Just spotted couple of last things. If the examples are now working, looks good to me after these last fixes.

datapythonista · 2018-10-27T10:45:49Z

pandas/core/tools/timedeltas.py

+    Convert argument to timedelta.
+
+    Timedeltas are absolute differences in times, expressed in difference
+    units e.g. (days, hours, minutes, seconds). This method converts an argument


I'd move the e.g. inside the parenthesis I think that makes more sense.

datapythonista · 2018-10-27T10:56:45Z

pandas/core/tools/timedeltas.py

-        - If True returns a Timedelta/TimedeltaIndex of the results
-        - if False returns a np.timedelta64 or ndarray of values of dtype
-          timedelta64[ns]
+    arg : str, timedelta, list, tuple, 1-d array, or Series


I'd replace list, tuple, 1-d array by list-like. I'd also remove the comma vefore the or.

ryankarlos · 2018-10-27T14:06:51Z

@datapythonista done :)

datapythonista

lgtm, thanks!

ryankarlos · 2018-11-11T02:28:39Z

@datapythonista There is a now a conflict with the most recent merge #23439 - with more additions in the units parameters. Should I revert to the version we have in this - or do you prefer to include all those units ('Y', 'M' shouldn't be in there for eg ?). Let me know and I can fix.

datapythonista · 2018-11-11T22:14:32Z

Unlucky this was not merged earlier.

You need to fix the conflict in a way that you get the lastest changes (from the conflicting PR in this case), and you make any fixes to that.

This PR is a diff of what you have and master, if you take your changes, you'll be deleting the work in that PR here.

I'm not sure of how the long set of units in that PR renders. If it doesn't render correctly, you can fix it here (instead of having the units in the type, just say str and then you show the units as a list in the description).

ryankarlos · 2018-11-11T23:18:02Z

@datapythonista ok thanks - ill fetch the latest changes from master - so is it ok for me to list the units as the short format (D, h, m, s, ms, us, ns) in the description if the long format looks odd on rendering. Is expanding on all the different unit formats necessary ?

datapythonista · 2018-11-11T23:20:34Z

Yes, if they were added that's for a reason. Avoid changing the content unless you really know what you're doing. But make the format look as good as possible. And in this case, I'm not sure how sphinx handles multilines in the types, we had problems with that in the past, so may be the current format is displayed broken.

ryankarlos · 2018-11-13T00:13:04Z

@datapythonista Ive fixed the merge conflict to include changes from upstream. The units were not rendering properly when including in the type (the format was getting broken after the first line) so I shifted it to the description as you had suggested.

datapythonista

Sorry for the delay in reviewing.

Looks good, just couple of minor things, and I think it's ready to be merged.

pandas/core/tools/timedeltas.py

jreback · 2018-11-20T02:09:26Z

@datapythonista over to you. merge when satisfied.

datapythonista · 2018-11-20T10:26:47Z

Thanks for fixing this @ryankarlos

jreback requested changes Oct 20, 2018

View reviewed changes

pandas/core/tools/timedeltas.py Outdated Show resolved Hide resolved

mroeschke reviewed Oct 21, 2018

View reviewed changes

ryankarlos changed the title ~~DOC: Fixed timedelta docstring to include month and year units and examples~~ DOC: Fixed timedelta docstring examples Oct 21, 2018

ryankarlos changed the title ~~DOC: Fixed timedelta docstring examples~~ DOC: Fixed timedelta docstring and examples Oct 21, 2018

ryankarlos force-pushed the timedelta_docstring branch from f112c2e to 88ff298 Compare October 21, 2018 13:01

mroeschke reviewed Oct 21, 2018

View reviewed changes

ryankarlos mentioned this pull request Oct 21, 2018

DEPR: Add Deprecated warning for timedelta with passed units M and Y #23264

Merged

4 tasks

jschendel added the Docs label Oct 21, 2018

datapythonista reviewed Oct 25, 2018

View reviewed changes

datapythonista reviewed Oct 27, 2018

View reviewed changes

datapythonista approved these changes Oct 27, 2018

View reviewed changes

sinhrks mentioned this pull request Nov 1, 2018

BUG: Cleanup timedelta offset #23439

Merged

4 tasks

ryankarlos force-pushed the timedelta_docstring branch from 0c5129f to e6d45a7 Compare November 13, 2018 00:06

datapythonista requested changes Nov 19, 2018

View reviewed changes

pandas/core/tools/timedeltas.py Outdated Show resolved Hide resolved

pandas/core/tools/timedeltas.py Outdated Show resolved Hide resolved

pandas/core/tools/timedeltas.py Outdated Show resolved Hide resolved

ryankarlos added 7 commits November 20, 2018 00:22

Modified timedelta docstring to include M and Y units and examples

641e51c

Corrected summary

2e6a20b

Added box example and removed M and Y unit references

5e90441

Removed last example

ed3a711

improved styling

8b580bb

styling/linting

f85f0f6

Final fixes and Fixed merge conflict/included recent upstream changes

737ac0b

ryankarlos force-pushed the timedelta_docstring branch from 55b095a to 8777629 Compare November 20, 2018 00:27

Additional small fixes

a67649a

ryankarlos force-pushed the timedelta_docstring branch from 8777629 to a67649a Compare November 20, 2018 00:46

jreback added this to the 0.24.0 milestone Nov 20, 2018

jreback added the Timedelta Timedelta data type label Nov 20, 2018

jreback approved these changes Nov 20, 2018

View reviewed changes

datapythonista merged commit 83c7e28 into pandas-dev:master Nov 20, 2018

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

DOC: Updating to_timedelta docstring (pandas-dev#23259)

0642031

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

DOC: Updating to_timedelta docstring (pandas-dev#23259)

791f8f8

Uh oh!

Conversation

ryankarlos commented Oct 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pep8speaks commented Oct 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated on October 27, 2018 at 13:50 Hours UTC

Uh oh!

ryankarlos commented Oct 20, 2018

Uh oh!

ryankarlos commented Oct 20, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mroeschke Oct 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryankarlos Oct 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mroeschke commented Oct 25, 2018

Uh oh!

ryankarlos commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

datapythonista left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryankarlos commented Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

datapythonista left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ryankarlos commented Oct 20, 2018 •

edited

Loading

pep8speaks commented Oct 20, 2018 •

edited

Loading

mroeschke Oct 21, 2018 •

edited

Loading

ryankarlos Oct 21, 2018 •

edited

Loading

codecov bot commented Oct 25, 2018 •

edited

Loading

ryankarlos commented Oct 25, 2018 •

edited

Loading

ryankarlos commented Oct 26, 2018 •

edited

Loading

ryankarlos commented Oct 27, 2018 •

edited

Loading

ryankarlos commented Oct 27, 2018 •

edited

Loading

datapythonista left a comment •

edited

Loading