DOC: Updating pandas.Interval docstring #20057

akielbowicz · 2018-03-08T16:09:19Z

Docstring assigned to the Buenos Aires chapter for the sprint.

Hi, I have a few doubts about the changes.

First about the type of the parameters left and right, it's ok to refer to them as orderable?
In the implementation, the constructor takes any pair of objects that satisfy left <= right and the comparison returns a boolean value.
I tried to pass two numpy arrays and it breaks because of ambiguity, but still it's so general that you can even make Intervals of Intervals, sets or booleans. And I believe this is too permissive and prone to error.

So what it's better, put a warning about this or add an example that is a little more complex and uses this capability (I tried to think one, but nothing came up to my mind).

And last, validate_docstrings.py returned this error

2341, in _signature_from_callable 'no signature found for builtin type {!r}'.format(obj)) ValueError: no signature found for builtin type <class 'pandas._libs.interval.Interval'>

Thanks, happy to collaborate.

PS: Given that this is a Cython file I needed to recompile the project to see the changes in the documentation. I mention this because is not in the sprint guide and I don't know if I will able to add it before the event.

jorisvandenbossche

Thanks for the PR! Added some comments

jorisvandenbossche · 2018-03-08T17:46:59Z

pandas/_libs/interval.pyx

-        Left bound for the interval
-    right : value
-        Right bound for the interval
+    left : orderable object


maybe 'scalar' instead of object? To make clear an array-like is not accepted?

jorisvandenbossche · 2018-03-08T17:48:01Z

pandas/_libs/interval.pyx

+
+    Notes
+    -----
+    You must be able to compare the parameters **left** and **right**,


Can you use single backticks to refer to the keywords? (like `left`), and double backticks for the code snippet on the line below ?

jorisvandenbossche · 2018-03-08T17:53:08Z

pandas/_libs/interval.pyx

+    The examples below show how you can build Intervals of various types
+    of elements. In particular a numeric interval, a time interval and
+    finally a string interval. Given an Interval you can check whether 
+    an element belongs to it.


In general, I think it can be easier to follow to put some of those explanations between the examples, instead of first all explanation and then all examples.

jorisvandenbossche · 2018-03-08T17:55:42Z

And last, validate_docstrings.py returned this error

2341, in _signature_from_callable 'no signature found for builtin type {!r}'.format(obj)) ValueError: no signature found for builtin type <class 'pandas._libs.interval.Interval'>

Can you try the latest version? I think this should be fixed now.
You can update this branch by doing:

git fetch upstream
git merge upstream/master

and then try the script again.

Given that this is a Cython file I needed to recompile the project to see the changes in the documentation. I mention this because is not in the sprint guide and I don't know if I will able to add it before the event.

Ah, indeed, that's a good catch. That's something we need to add to the guide.

jorisvandenbossche · 2018-03-08T17:59:54Z

So what it's better, put a warning about this or add an example that is a little more complex and uses this capability (I tried to think one, but nothing came up to my mind).

I would personally give 'good examples', as you did now. And the note you added about left <= right needing to work seems sufficient to me.

jschendel · 2018-03-08T19:47:36Z

pandas/_libs/interval.pyx

    >>> year_2017 = pd.Interval(pd.Timestamp('2017-01-01'),
    ...                         pd.Timestamp('2017-12-31'), closed='both')
    >>> pd.Timestamp('2017-01-01 00:00') in year_2017
    True
+    >>> year_2017.length
+    Timedelta('364 days 00:00:00')


Might be nice to change year_2017 to

year_2017 = pd.Interval(pd.Timestamp('2017-01-01'), pd.Timestamp('2018-01-01'), closed='left')

That seems like a more accurate representation of a year, since the current version doesn't include anything beyond 2017-12-31 00:00:00, e.g. 2017-12-31 12:00:00 isn't included in year_2017 as currently written. This change would also result in year_2017.length being 365 days, which should be more intuitive for users.

…val_docstring Updating the advanced branch

…val_docstring Preparing for commit

codecov · 2018-03-10T02:48:34Z

Codecov Report

Merging #20057 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #20057      +/-   ##
==========================================
+ Coverage    91.7%   91.73%   +0.02%     
==========================================
  Files         150      150              
  Lines       49168    49168              
==========================================
+ Hits        45090    45102      +12     
+ Misses       4078     4066      -12

Flag	Coverage Δ
#multiple	`90.11% <ø> (+0.02%)`	⬆️
#single	`41.86% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/plotting/_converter.py	`66.81% <0%> (+1.73%)`	⬆️

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 df2e361...4682386. Read the comment docs.

akielbowicz · 2018-03-10T02:55:22Z

Thanks for the comments, I tried to improve it following your guidelines.

jorisvandenbossche · 2018-03-10T11:26:00Z

@jschendel could you give this another look?

jreback · 2018-03-10T12:40:08Z

pandas/_libs/interval.pyx


    Examples
    --------
+    It is possible to build Intervals of different types, like numeric ones


can you add that left & right must be of the same type. (add to Notes)

jreback · 2018-03-10T12:41:32Z

pandas/_libs/interval.pyx

+
+    >>> volume_1 = pd.Interval('Ant', 'Dog', closed='both')
+    >>> 'Bee' in volume_1
+    True

    See Also


can you add Period here as well (its pretty much a time based interval)

To add a couple of points here: Interval is basically a more generic Period, in that Period requires a frequency and Interval does not. The trade-off being that Period has additional frequency specific functionality that Interval does not.

jschendel

Thanks! LGTM pending @jreback's comments. Added one comment regarding an optional potential addition that could be made.

jschendel · 2018-03-10T21:08:05Z

pandas/_libs/interval.pyx

+    >>> extended_iv = iv * 10.0
+    >>> extended_iv
+    Interval(0.0, 50.0, closed='right')
+


Optional: Could maybe add that an interval does not necessarily need to be bounded, e.g. pd.Interval(-np.inf, 5). Not sure if there's a ton of utility in adding this though.

…val_docstring

jorisvandenbossche · 2018-03-14T13:11:59Z

@akielbowicz Thanks a lot!

Added type to the parameter and extend the examples

eb8ce9f

jorisvandenbossche added the Docs label Mar 8, 2018

jorisvandenbossche reviewed Mar 8, 2018

View reviewed changes

jschendel reviewed Mar 8, 2018

View reviewed changes

akielbowicz added 4 commits March 9, 2018 21:01

Merge remote-tracking branch 'upstream/master' into DOC_improve_Inter…

e8cbf0b

…val_docstring Updating the advanced branch

Changed referece to parameters with sigle backticks

080654d

Merge remote-tracking branch 'upstream/master' into DOC_improve_Inter…

f302acc

…val_docstring Preparing for commit

Improved the examples and replaced * to backticks

713d975

jreback requested changes Mar 10, 2018

View reviewed changes

jreback added the Interval Interval data type label Mar 10, 2018

jorisvandenbossche mentioned this pull request Mar 10, 2018

DOC: update the docstrings of Interval and IntervalMixin #20132

Merged

5 tasks

jschendel reviewed Mar 10, 2018

View reviewed changes

akielbowicz and others added 6 commits March 10, 2018 18:27

Merge remote-tracking branch 'upstream/master' into DOC_improve_Inter…

a0db247

…val_docstring

Update notes

7db6817

Merge remote-tracking branch 'upstream/master' into DOC_improve_Inter…

5a6727a

…val_docstring

Added Period to See Also

c1d7265

Merge remote-tracking branch 'upstream/master' into DOC_improve_Inter…

45f7f3b

…val_docstring

Update interval.pyx

4682386

jorisvandenbossche approved these changes Mar 14, 2018

View reviewed changes

jorisvandenbossche merged commit 921fefb into pandas-dev:master Mar 14, 2018

jorisvandenbossche added this to the 0.23.0 milestone Mar 14, 2018

Uh oh!

DOC: Updating pandas.Interval docstring #20057

DOC: Updating pandas.Interval docstring #20057

Uh oh!

Conversation

akielbowicz commented Mar 8, 2018

Uh oh!

jorisvandenbossche 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!

jorisvandenbossche commented Mar 8, 2018

Uh oh!

jorisvandenbossche commented Mar 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

akielbowicz commented Mar 10, 2018

Uh oh!

jorisvandenbossche commented Mar 10, 2018

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!

jschendel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche commented Mar 14, 2018

Uh oh!

Uh oh!

codecov bot commented Mar 10, 2018 •

edited

Loading