DOC: Clean sort_values and sort_index docstrings

[dsaxton]

Fixes some capitalization

[codecov]

Codecov Report

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

@@           Coverage Diff           @@
##           master   #24843   +/-   ##
=======================================
  Coverage   92.39%   92.39%           
=======================================
  Files         166      166           
  Lines       52378    52378           
=======================================
  Hits        48393    48393           
  Misses       3985     3985

Flag	Coverage Δ
#multiple	90.81% <ø> (ø)	⬆️
#single	42.9% <ø> (ø)	⬆️

Impacted Files	Coverage Δ
pandas/core/generic.py	96.62% <ø> (ø)	⬆️

---

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 f4458c1...0f5e99f. Read the comment docs.

[codecov]

Codecov Report

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

@@            Coverage Diff             @@
##           master   #24843      +/-   ##
==========================================
- Coverage   92.38%   92.37%   -0.01%     
==========================================
  Files         166      166              
  Lines       52413    52388      -25     
==========================================
- Hits        48424    48396      -28     
- Misses       3989     3992       +3

Flag	Coverage Δ
#multiple	90.8% <ø> (-0.02%)	⬇️
#single	42.9% <ø> (ø)	⬆️

Impacted Files	Coverage Δ
pandas/core/series.py	93.68% <ø> (ø)	⬆️
pandas/core/generic.py	96.63% <ø> (ø)	⬆️
pandas/core/nanops.py	93.84% <0%> (-0.53%)	⬇️
pandas/plotting/_core.py	83.58% <0%> (-0.18%)	⬇️
pandas/core/reshape/merge.py	94.4% <0%> (-0.07%)	⬇️
pandas/io/formats/format.py	97.98% <0%> (-0.01%)	⬇️
pandas/core/indexes/interval.py	95.25% <0%> (ø)	⬆️
pandas/core/api.py	100% <0%> (ø)	⬆️
pandas/core/arrays/integer.py	96.32% <0%> (ø)	⬆️
pandas/core/arrays/interval.py	93.12% <0%> (ø)	⬆️
... and 2 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 2fa0835...75750da. Read the comment docs.

[gfyoung]

@datapythonista : Just curious, do we not lint docstrings to check for capitalization like this?

[WillAyd]

Can you run scripts/validate_docstrings.py against these and post output? Think there are other issues there as well so makes sense to update all while touching

[WillAyd]

@gfyoung yep that's part of the existing validator. Below is output from master for me:

################################################################################
############# Docstring (pandas.core.generic.NDFrame.sort_values)  #############
################################################################################

Sort by the values along either axis

Parameters
----------%(optional_by)s
axis : %(axes_single_arg)s, default 0
     Axis to be sorted
ascending : bool or list of bool, default True
     Sort ascending vs. descending. Specify list for multiple sort
     orders.  If this is a list of bools, must match the length of
     the by.
inplace : bool, default False
     if True, perform operation in-place
kind : {'quicksort', 'mergesort', 'heapsort'}, default 'quicksort'
     Choice of sorting algorithm. See also ndarray.np.sort for more
     information.  `mergesort` is the only stable algorithm. For
     DataFrames, this option is only applied when sorting on a single
     column or label.
na_position : {'first', 'last'}, default 'last'
     `first` puts NaNs at the beginning, `last` puts NaNs at the end

Returns
-------
sorted_obj : %(klass)s

Examples
--------
>>> df = pd.DataFrame({
...     'col1' : ['A', 'A', 'B', np.nan, 'D', 'C'],
...     'col2' : [2, 1, 9, 8, 7, 4],
...     'col3': [0, 1, 9, 4, 2, 3],
... })
>>> df
    col1 col2 col3
0   A    2    0
1   A    1    1
2   B    9    9
3   NaN  8    4
4   D    7    2
5   C    4    3

Sort by col1

>>> df.sort_values(by=['col1'])
    col1 col2 col3
0   A    2    0
1   A    1    1
2   B    9    9
5   C    4    3
4   D    7    2
3   NaN  8    4

Sort by multiple columns

>>> df.sort_values(by=['col1', 'col2'])
    col1 col2 col3
1   A    1    1
0   A    2    0
2   B    9    9
5   C    4    3
4   D    7    2
3   NaN  8    4

Sort Descending

>>> df.sort_values(by='col1', ascending=False)
    col1 col2 col3
4   D    7    2
5   C    4    3
2   B    9    9
0   A    2    0
1   A    1    1
3   NaN  8    4

Putting NAs first

>>> df.sort_values(by='col1', ascending=False, na_position='first')
    col1 col2 col3
3   NaN  8    4
4   D    7    2
5   C    4    3
2   B    9    9
0   A    2    0
1   A    1    1

################################################################################
################################## Validation ##################################
################################################################################

10 Errors found:
	Summary does not end with a period
	Parameters {by} not documented
	Parameter "axis" description should finish with "."
	Parameter "inplace" description should start with a capital letter
	Parameter "inplace" description should finish with "."
	Parameter "na_position" description should start with a capital letter
	Parameter "na_position" description should finish with "."
	The first line of the Returns section should contain only the type, unless multiple values are being returned
	Return value has no description
	flake8 error: E203 whitespace before ':' (2 times)
2 Warnings found:
	No extended summary found
	See Also section not found

Might as well get all of these cleaned up while touching

[gfyoung]

@WillAyd : If that's the case, wouldn't the CI have failed then?

[WillAyd]

I believe we have a whitelist of error codes at the moment. The issues with this docstring wouldn't trigger a CI failure due to that but should nonetheless get cleaned up as we expand the whitelist

pandas/ci/code_checks.sh

Line 243 in 402d5db

MSG='Validate docstrings (GL06, GL07, GL09, SS04, PR03, PR05, EX04)' ; echo $MSG

[datapythonista]

I think before we can validate this error code (capitalization of the parameters description), we still need to change the validation to not report false positives when a parameter description starts with a list, and we also need to fix lots of cases that are incorrect.

[dsaxton]

@datapythonista Yes, the na_position error for instance seems to be a false positive because the description starts with one of the options for that argument. Also, I've noticed that a lot of pandas documentation seems to follow a convention that single sentence descriptions don't need to be ended with a period, so lots of docstrings would likely be flagged here

[WillAyd]

Looking better. Slight rewording will reduce errors but still convey same message. Saw a few more on the validation that should be an easy fixup

[pep8speaks]

Hello @dsaxton! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on January 26, 2019 at 00:59 Hours UTC

[WillAyd]

Can you tackle remaining issues?

5 Errors found:
	Summary does not end with a period
	Parameters {by} not documented
	The first line of the Returns section should contain only the type, unless multiple values are being returned
	Return value has no description
	flake8 error: E203 whitespace before ':' (2 times)

First two are pretty clear. I think the last three may all be resolved if you add a description to the return object

[dsaxton]

@WillAyd I got the first one, but I'm not sure how the others might work when the docstring is a generic one covering different classes (e.g., the Series doesn't have a by argument):

        """
        Sort by the values along either axis.

        Parameters
        ----------%(optional_by)s
        axis : %(axes_single_arg)s, default 0
             Axis to be sorted.
        ascending : bool or list of bool, default True
             Sort ascending vs. descending. Specify list for multiple sort
             orders.  If this is a list of bools, must match the length of
             the by.
        inplace : bool, default False
             If True, perform operation in-place.
        kind : {'quicksort', 'mergesort', 'heapsort'}, default 'quicksort'
             Choice of sorting algorithm. See also ndarray.np.sort for more
             information.  `mergesort` is the only stable algorithm. For
             DataFrames, this option is only applied when sorting on a single
             column or label.
        na_position : {'first', 'last'}, default 'last'
             `first` puts NaNs at the beginning, `last` puts NaNs at the end.

        Returns
        -------
        sorted_obj : %(klass)s

[WillAyd]

Hmm it doesn't look like the docstrings are actually shared though? Looks like Series is creating its own so I don't think edits here affect that (though some cleanup / consolidation could happen in a separate PR)

Side note - if you merge master instead of rebasing shouldn't need to force push. Makes review process easier as it maintains comments better

[dsaxton]

Hmm it doesn't look like the docstrings are actually shared though? Looks like Series is creating its own so I don't think edits here affect that (though some cleanup / consolidation could happen in a separate PR)

Side note - if you merge master instead of rebasing shouldn't need to force push. Makes review process easier as it maintains comments better

Got it, will merge instead in the future

[dsaxton]

@WillAyd Added a tiny fix to the Series method. Was there anything else you'd like to wrap into this PR?

[WillAyd]

Can you post the output of scripts/validate_docstrings.py pandas.core.generic.NDFrame.sort_<FUNC>?

[dsaxton]

sort_values:

################################################################################
################################## Validation ##################################
################################################################################

4 Errors found:
	Parameters {by} not documented
	The first line of the Returns section should contain only the type, unless multiple values are being returned
	Return value has no description
	flake8 error: E203 whitespace before ':' (2 times)
2 Warnings found:
	No extended summary found
	See Also section not found

sort_index:

################################################################################
################################## Validation ##################################
################################################################################

5 Errors found:
	Parameter "axis" has no description
	Parameter "ascending" type should use "bool" instead of "boolean"
	Parameter "inplace" description should finish with "."
	The first line of the Returns section should contain only the type, unless multiple values are being returned
	Return value has no description
3 Warnings found:
	No extended summary found
	See Also section not found
	No examples section found

[WillAyd]

Thanks! Can you fix remaining errors?

[dsaxton]

I'm not quite sure how to fix the "Returns" errors given the generic nature of the docstring (same for the optional by). Also for sort_index the axis argument does seem to have a description, so I'm a bit confused on that one. Any ideas?

[dsaxton]

There was one other error in Series.sort_index, but should be good now

[WillAyd]

Check the documentation for set_axis in the same module could mimic or even copy the returns and axis parts there from

…

Sent from my iPhone

On Jan 23, 2019, at 8:34 PM, Daniel Saxton ***@***.***> wrote: There was one other error in Series.sort_index, but should be good now — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[dsaxton]

Added a couple more fixes. It looks though that the set_axis method is also giving an error:

################################################################################
################################## Validation ##################################
################################################################################

1 Errors found:
	The first line of the Returns section should contain only the type, unless multiple values are being returned

Here's the returns section:

        Returns
        -------
        renamed : %(klass)s or None
            An object of same type as caller if inplace=False, None otherwise.

[dsaxton]

(Force push there was just to fix a commit message typo)

[WillAyd]

OK so the returns section couldn't be copied verbatim. We aren't using substitution so %(klass)s isn't valid.

FYI I think your issue is the or None piece (wrong in docstring you copied from) but would be moot point with suggested change.

[dsaxton]

OK so the returns section couldn't be copied verbatim. We aren't using substitution so %(klass)s isn't valid.

FYI I think your issue is the or None piece (wrong in docstring you copied from) but would be moot point with suggested change.

Okay, I see what you mean about the klass part. Isn't it true though that the methods return None when an in-place operation is done?

[WillAyd]

Valid point - feel free to keep the None then and we can ignore that issue / merge after update

…

Sent from my iPhone

On Jan 24, 2019, at 5:53 AM, Daniel Saxton ***@***.***> wrote: OK so the returns section couldn't be copied verbatim. We aren't using substitution so %(klass)s isn't valid. FYI I think your issue is the or None piece (wrong in docstring you copied from) but would be moot point with suggested change. Okay, I see what you mean about the klass part. Isn't it true though that the methods return None when an in-place operation is done? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[dsaxton]

Pushed another commit. I noticed we were actually editing the sort_index returns section so I reworded "sorted values" as "sorted index."

Sorry, but did we want to make a similar change to sort_values regarding the klass substitution? I think only the DataFrame will use this method?

[WillAyd]

Makes sense for consistency

…

Sent from my iPhone

On Jan 24, 2019, at 6:09 PM, Daniel Saxton ***@***.***> wrote: Pushed another commit. I noticed we were actually editing the sort_index returns section so I reworded "sorted values" as "sorted index." Sorry, but did we want to make a similar change to sort_values regarding the klass substitution? I think only the DataFrame will use this method? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[WillAyd]

I suppose its just worth moving these docstrings to the frame class, since they aren't actually used anywhere else. If that's the case certainly lgtm (and thanks for sticking through all the edits!)

@jreback - do I need to milestone this PR any particular way before merging?

[jreback]

0.25.0

[dsaxton]

I suppose its just worth moving these docstrings to the frame class, since they aren't actually used anywhere else. If that's the case certainly lgtm (and thanks for sticking through all the edits!)

@WillAyd NP, thanks for the help!

[WillAyd]

@dsaxton on second thought this looks good as is. We could still refine this by actually using substitution (knowing there's a slight difference in signature) but can do in a separate PR if you want

Thanks again!