Issue/26506 Provides correct desciption in docstring that get_indexer methods are not yet supported #26519

timgates42 · 2019-05-25T01:58:28Z

fixes docstring for IntervalIndex.get_indexer "method" options are not implemented but documentation implies they are #26506 until implementation is ready
46585 tests passed - none added as documentation change only
passes git diff upstream/master -u -- "*.py" | flake8 --diff

$ git diff upstream/master -u -- "*.py" | flake8 --diff && echo yes
yes

whatsnew entry
Provides correct desciption in docstring that Interval.get_indexer using any method other than default are not yet supported

… so will back out and follow that pattern

Originally had made a template from the documentation so a common basis could be used with the IntervalIndex adding a warning that the other methods were as yet unsupported. In the end because I am not clear on what effect putting a template in the core documentation dictionary I feel back to just following get_loc and forking the doc string.

timgates42 · 2019-05-25T02:00:13Z

I followed the example from get_loc to adjust the documentation, I originally had created a template the each documentation strings used as a basis but was concerned how it would interact with the global documentation dictionary and how it was used. Let me know if you would like any changes.

timgates42 · 2019-05-25T02:36:08Z

Oh wow - found the right way to do it in the Contributors guide my apologies for rushing into it without checking the great documentation - I will fix the PR to follow the recomendation in https://pandas-docs.github.io/pandas-docs-travis/development/contributing_docstring.html#sharing-docstrings

codecov · 2019-05-25T02:39:48Z

Codecov Report

Merging #26519 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #26519      +/-   ##
==========================================
- Coverage   91.75%   91.74%   -0.01%     
==========================================
  Files         174      174              
  Lines       50665    50664       -1     
==========================================
- Hits        46489    46484       -5     
- Misses       4176     4180       +4

Flag	Coverage Δ
#multiple	`90.26% <ø> (-0.01%)`	⬇️
#single	`41.71% <ø> (-0.09%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/interval.py	`96.09% <ø> (-0.01%)`	⬇️
pandas/io/gbq.py	`78.94% <0%> (-10.53%)`	⬇️
pandas/core/frame.py	`97% <0%> (-0.12%)`	⬇️

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 ba48fc4...b7fc1bd. Read the comment docs.

codecov · 2019-05-25T02:39:49Z

Codecov Report

Merging #26519 into master will decrease coverage by 50.06%.
The diff coverage is 12.5%.

@@             Coverage Diff             @@
##           master   #26519       +/-   ##
===========================================
- Coverage   91.75%   41.69%   -50.07%     
===========================================
  Files         174      174               
  Lines       50665    50643       -22     
===========================================
- Hits        46489    21115    -25374     
- Misses       4176    29528    +25352

Flag	Coverage Δ
#multiple	`?`
#single	`41.69% <12.5%> (-0.11%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/base.py	`55.72% <ø> (-40.99%)`	⬇️
pandas/core/indexes/interval.py	`34.54% <12.5%> (-61.57%)`	⬇️
pandas/io/formats/latex.py	`0% <0%> (-100%)`	⬇️
pandas/io/sas/sas_constants.py	`0% <0%> (-100%)`	⬇️
pandas/core/groupby/categorical.py	`0% <0%> (-100%)`	⬇️
pandas/tseries/plotting.py	`0% <0%> (-100%)`	⬇️
pandas/tseries/converter.py	`0% <0%> (-100%)`	⬇️
pandas/io/formats/html.py	`0% <0%> (-99.37%)`	⬇️
pandas/io/sas/sas7bdat.py	`0% <0%> (-91.16%)`	⬇️
pandas/io/sas/sas_xport.py	`0% <0%> (-90.1%)`	⬇️
... and 130 more

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 ba48fc4...ae7470c. Read the comment docs.

Following the notes in the contributors guide - have opted for using docstring substition (https://pandas-docs.github.io/pandas-docs-travis/development/contributing_docstring.html#sharing-docstrings).

WillAyd

I think it would be easier to just append a Raises section to the doctoring for IntervalIndex

WillAyd · 2019-05-26T06:31:45Z

cc @jschendel

timgates42 · 2019-05-26T06:34:35Z

Thanks - good suggestion will do

@WillAyd

As suggested by @WillAyd instead of altering the docstring using a global substitution just append a raises docstring section that warns about the parts not yet implemented.

timgates42 · 2019-05-26T10:09:51Z

Thanks @WillAyd it was much easier - have modified it to append a raises section to the docstring as requested.

Raises section must appear before Examples which complicates the use of just appending a doc section and instead must perform a substitution.

pep8speaks · 2019-05-26T11:15:46Z

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-05-28 22:40:21 UTC

timgates42 · 2019-05-26T11:16:51Z

Ran into a complication with the plan as the Raises section must be before the Examples in the docstring so hopefully this Substitution technique is the appropriate answer.

Avoid exceeding column count and maintain flake8 line overflow requirements

TomAugspurger · 2019-05-26T12:03:18Z

pandas/core/indexes/interval.py

+                       **{'raises_section': textwrap.dedent("""\
+        Raises
+        ------
+        raises NotImplementedError if any method other than than the default


Typically this is formatted like

Raises ------ ExceptionName Condition.

Thanks @TomAugspurger - have adjusted accordingly

If you supply an argument then it assumes the substitution is a straight sequence for the string format as it uses args or kwargs.

pandas/core/indexes/interval.py

pandas/core/indexes/base.py

@jreback

If we remove the line at the end when substituting in the raises section and add one at the start then in the template the raises_section substitution can be on its own line which is a cleaner solution - thanks to @jreback.

WillAyd · 2019-05-26T20:22:47Z

@timgates42 I think this looks generally good. Can you post a screenshot of the rendered HTML doc?

timgates42 · 2019-05-27T12:08:25Z

@timgates42 I think this looks generally good. Can you post a screenshot of the rendered HTML doc?

Sure @WillAyd here it is

WillAyd · 2019-05-27T14:38:37Z

The Raises section looks off with the asterisks - do you know why that is appearing?

…

Sent from my iPhone

On May 27, 2019, at 5:08 AM, Tim Gates ***@***.***> wrote: @timgates42 I think this looks generally good. Can you post a screenshot of the rendered HTML doc? Sure @WillAyd here it is — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jreback · 2019-05-27T14:54:15Z

ahh, just noticed @WillAyd comment, can you update.

timgates42 · 2019-05-27T20:01:20Z

Good catch - according to the sphinx documentation this comes out if you are missing a colon (which I am) - I’ll add and regenerate - I actually copied this from another spot in pandas so presumably that needs fixing too - will find it

timgates42 · 2019-05-28T00:05:22Z

Oh interesting I found the function I copied rename_categories in pandas/core/arrays/categorical.py and the issue can be seen in the published doc http://pandas-docs.github.io/pandas-docs-travis/reference/api/pandas.CategoricalIndex.rename_categories.html?highlight=rename_categories#pandas.CategoricalIndex.rename_categories but the fix is not what I expected as pandas uses numpydoc a varient of sphinx doc and it looks like this raises syntax is common across pandas as is the strange asterisks sequence - http://pandas-docs.github.io/pandas-docs-travis/reference/api/pandas.api.types.union_categoricals.html?highlight=union_categoricals#pandas.api.types.union_categoricals

timgates42 · 2019-05-28T00:06:18Z

I do get these warnings when producing the documentation which is the next lead to track down...

home/tgates/3rdparty/pandas/pandas/core/indexes/interval.py:docstring of pandas.IntervalIndex.get_indexer:54: WARNING: Inline strong start-string without end-string.
WARNING: toctree references unknown document 'reference/api/pandas.RangeIndex.start'
WARNING: toctree references unknown document 'reference/api/pandas.RangeIndex.stop'
WARNING: toctree references unknown document 'reference/api/pandas.RangeIndex.step'
WARNING: toctree references unknown document 'reference/api/pandas.Series.sparse'

WillAyd

@timgates42 thanks for the research. Given this is a pre-existing issue across the code base I'm OK with this PR as is and would take a follow up PR to fix the asterisks.

Of course up to @jreback as well

timgates42 · 2019-05-28T22:36:00Z

Oh just spotted I had doubled up than and it was any method other than than the default, I'll fix it.

simonjayhawkins · 2019-05-29T01:03:17Z

Given this is a pre-existing issue across the code base I'm OK with this PR as is and would take a follow up PR to fix the asterisks.

without checking explicitly, it may be related to #26058, there seem to be a few issues regarding formatting. The dev docs at http://pandas-docs.github.io/pandas-docs-travis are being built on Travis using Sphinx v2.0.1

timgates42 · 2019-05-29T06:01:10Z

Oh thanks for the heads up @simonjayhawkins I can confirm I built locally using sphinx v2.0.1 let me check 1.8.5 to see if it looks better.

timgates42 · 2019-05-29T23:19:51Z

Rolling back to sphinx 1.8.5 didn't solve the issue, I'll continue investigation. I've setup a separate repo at https://github.com/timgates42/numpydoc-demo to run some testing of numpydoc to see if I can reproduce the issue. If I solve it then I'm happy to send another PR as its unrelated to the problem being addressed here. Let me know if anyone disagrees or wants a change to this PR otherwise assuming all is good please merge it if this is appropriate - Thanks.

jreback · 2019-05-30T01:28:19Z

thanks @timgates42

if you can create an issue for the astericks thing (you can just refer back to the comments here), and a PR to fix would be great!

timgates42 added 3 commits May 25, 2019 08:34

Started work on shared doc idea but noticed get_loc is already forked…

6be1235

… so will back out and follow that pattern

Add back in note to adjust the doc string if fixing the implementation

b7fc1bd

timgates42 changed the title ~~Issue/26506~~ Issue/26506 Provides correct desciption in docstring that get_indexer methods are not yet supported May 25, 2019

timgates42 added 2 commits May 25, 2019 12:48

Use docstring substitution instead

9b9436f

Following the notes in the contributors guide - have opted for using docstring substition (https://pandas-docs.github.io/pandas-docs-travis/development/contributing_docstring.html#sharing-docstrings).

Remove double full-stop

1777120

gfyoung added Docs Indexing Related to indexing on series/frames, not to indexes themselves Interval Interval data type labels May 25, 2019

gfyoung requested a review from jreback May 25, 2019 02:59

timgates42 added 2 commits May 25, 2019 13:33

Looks like this section has its own substition technique

7f06fbd

Drop unused import

810ec5a

WillAyd requested changes May 26, 2019

View reviewed changes

timgates42 added 3 commits May 26, 2019 19:55

Restore original

0ad7c8c

Add back in note to adjust the doc string if fixing the implementation

f6331e4

Use Appender raises to notify of the deficiency

3f83d82

As suggested by @WillAyd instead of altering the docstring using a global substitution just append a raises docstring section that warns about the parts not yet implemented.

Resolve check_doc failure

88f59c3

Raises section must appear before Examples which complicates the use of just appending a doc section and instead must perform a substitution.

Satisfy flake8 overflow and line length

dd6a18e

Avoid exceeding column count and maintain flake8 line overflow requirements

TomAugspurger reviewed May 26, 2019

View reviewed changes

timgates42 added 2 commits May 26, 2019 22:10

Use correct raises syntax - Thanks @TomAugspurger

a3fc35a

Substitution uses kwargs for dict formats.

c420a42

If you supply an argument then it assumes the substitution is a straight sequence for the string format as it uses args or kwargs.

jreback requested changes May 26, 2019

View reviewed changes

pandas/core/indexes/interval.py Outdated Show resolved Hide resolved

pandas/core/indexes/base.py Outdated Show resolved Hide resolved

timgates42 added 2 commits May 27, 2019 05:52

Drop comment as requested by @jreback

cd71f2c

Cleaner look as suggested by @jreback

4bbcbd1

If we remove the line at the end when substituting in the raises section and add one at the start then in the template the raises_section substitution can be on its own line which is a cleaner solution - thanks to @jreback.

jreback added this to the 0.25.0 milestone May 27, 2019

WillAyd approved these changes May 28, 2019

View reviewed changes

timgates42 added 2 commits May 29, 2019 08:37

Fix up use incorrect sentence that had a double "than" in it.

11e8a76

Fix layout to match expected raises syntax

ae7470c

jreback approved these changes May 30, 2019

View reviewed changes

jreback merged commit 59df3e0 into pandas-dev:master May 30, 2019

jreback mentioned this pull request May 30, 2019

IntervalIndex.get_indexer "method" options are not implemented but documentation implies they are #26506

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/26506 Provides correct desciption in docstring that get_indexer methods are not yet supported #26519

Issue/26506 Provides correct desciption in docstring that get_indexer methods are not yet supported #26519

timgates42 commented May 25, 2019 •

edited

Loading

timgates42 commented May 25, 2019

timgates42 commented May 25, 2019 •

edited

Loading

codecov bot commented May 25, 2019

codecov bot commented May 25, 2019 •

edited

Loading

WillAyd left a comment

WillAyd commented May 26, 2019

timgates42 commented May 26, 2019

timgates42 commented May 26, 2019

pep8speaks commented May 26, 2019 •

edited

Loading

timgates42 commented May 26, 2019

TomAugspurger May 26, 2019

timgates42 May 26, 2019

WillAyd commented May 26, 2019

timgates42 commented May 27, 2019

WillAyd commented May 27, 2019 via email

jreback commented May 27, 2019

timgates42 commented May 27, 2019

timgates42 commented May 28, 2019 •

edited

Loading

timgates42 commented May 28, 2019

WillAyd left a comment

timgates42 commented May 28, 2019

simonjayhawkins commented May 29, 2019

timgates42 commented May 29, 2019

timgates42 commented May 29, 2019

jreback commented May 30, 2019

Issue/26506 Provides correct desciption in docstring that get_indexer methods are not yet supported #26519

Issue/26506 Provides correct desciption in docstring that get_indexer methods are not yet supported #26519

Conversation

timgates42 commented May 25, 2019 • edited Loading

timgates42 commented May 25, 2019

timgates42 commented May 25, 2019 • edited Loading

codecov bot commented May 25, 2019

Codecov Report

codecov bot commented May 25, 2019 • edited Loading

Codecov Report

WillAyd left a comment

Choose a reason for hiding this comment

WillAyd commented May 26, 2019

timgates42 commented May 26, 2019

timgates42 commented May 26, 2019

pep8speaks commented May 26, 2019 • edited Loading

Comment last updated at 2019-05-28 22:40:21 UTC

timgates42 commented May 26, 2019

TomAugspurger May 26, 2019

Choose a reason for hiding this comment

timgates42 May 26, 2019

Choose a reason for hiding this comment

WillAyd commented May 26, 2019

timgates42 commented May 27, 2019

WillAyd commented May 27, 2019 via email

jreback commented May 27, 2019

timgates42 commented May 27, 2019

timgates42 commented May 28, 2019 • edited Loading

timgates42 commented May 28, 2019

WillAyd left a comment

Choose a reason for hiding this comment

timgates42 commented May 28, 2019

simonjayhawkins commented May 29, 2019

timgates42 commented May 29, 2019

timgates42 commented May 29, 2019

jreback commented May 30, 2019

timgates42 commented May 25, 2019 •

edited

Loading

timgates42 commented May 25, 2019 •

edited

Loading

codecov bot commented May 25, 2019 •

edited

Loading

pep8speaks commented May 26, 2019 •

edited

Loading

timgates42 commented May 28, 2019 •

edited

Loading