DOC: fix PR02 errors in docstrings - pandas.core.groupby.SeriesGroupBy.idxmax, pandas.core.groupby.SeriesGroupBy.idxmin

[jordan-d-murphy]

All PR02 Errors resolved in the following cases:

1. scripts/validate_docstrings.py --format=actions --errors=PR02 pandas.core.groupby.SeriesGroupBy.idxmax
2. scripts/validate_docstrings.py --format=actions --errors=PR02 pandas.core.groupby.SeriesGroupBy.idxmin

OUTPUT:

1. scripts/validate_docstrings.py --format=actions --errors=PR02 pandas.core.groupby.SeriesGroupBy.idxmax

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

Docstring for "pandas.core.groupby.SeriesGroupBy.idxmax" correct. :)

2. scripts/validate_docstrings.py --format=actions --errors=PR02 pandas.core.groupby.SeriesGroupBy.idxmin

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

Docstring for "pandas.core.groupby.SeriesGroupBy.idxmin" correct. :)

• xref DOC: fix PR02 errors in docstrings #57111
• Tests added and passed if fixing a bug or adding a new feature
• All code checks passed.
• Added type annotations to new arguments/methods/functions.
• Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

[datapythonista]

What's the reason to stop reusing the docstring here? Seems like the parameters are the same, no?

[jordan-d-murphy]

@datapythonista the reason I removed @doc(Series.idxmin.__doc__) and @doc(Series.idxmax.__doc__) is that Series.idxmin and Series.idxmax have extra parameters *args and **kwargs that were causing the PR02 errors when the same docstring was used for SeriesGroupBy.idxmin and SeriesGroupBy.idxmax (as these do not use *args and **kwargs).

I am open to other suggestions for resolving the PR02 errors if you know of other ways I can resolve this.

[jordan-d-murphy]

@mroeschke can you take a look at the examples I added?

[datapythonista]

the reason I removed @doc(Series.idxmin.__doc__) and @doc(Series.idxmax.__doc__) is that Series.idxmin and Series.idxmax have extra parameters *args and **kwargs

I see. Sorry, I compared the one changed here with the one in DataFrame and they have the same signature. I see it's the one with Series that has a different signature.

The way we've been dealing with this is using variables for the parts that change. In this case you could create a new variable extra_args which is empty in one case, and contains the docstring part of *args and **kwargs when needed. It's difficult to tell at what point reusing docstrings make things easier or more difficult, but for this case it may make sense if we are already reusing the docstrings.

[mroeschke]

IMO I am generally in favor of moving away from dynamic docstring. Especially since docstrings are a good avenue for new contributions, I think the overhead of doc/Appender/Substitution can make fixing docstrings more onerous. Also templating can get in the way of automatic docstring formatting that ruff now provides: #56863

[rhshadrach]

IMO I am generally in favor of moving away from dynamic docstring

+1

[rhshadrach]

It's difficult to tell at what point reusing docstrings make things easier or more difficult, but for this case it may make sense if we are already reusing the docstrings.

You would also have to do something about the different examples, as now the groupby version has groupby-specific examples.

[jordan-d-murphy]

@datapythonista considering the input above, is the effort to refactor this (main docstring + additional args + examples) worth undertaking at this time, or do we want to revisit that in a separate conversation outside of the scope of this PR?

There are other similar PR02 errors that could be resolved with either approach.

[datapythonista]

I was just pointing out at how we've been doing things so far. And as I said, I really don't know when reusing docstrings simplify things, and when it makes them more complex, it's probably more a personal preference than something we can agree with, and I don't have a strong opinion. So, I'm surely happy to let you decide how to move forward here.

[rhshadrach]

@jordan-d-murphy - when you merge main, you can remove any references to axis in your docstring as this argument has been removed.

[jordan-d-murphy]

Thank you all so much for the guidance and feedback. For now, I'll refrain from refactoring the docstring into the dynamic approach. I've merged main back into this branch to resolve conflicts, and the pipeline is running again. I also removed the axis references. Can someone take another look once the pipeline finishes and the checks are green?

[jordan-d-murphy]

@rhshadrach in this PR I've removed axis for SeriesGroupBy.idxmin and SeriesGroupBy.idxmax

I see Series.idxmin and Series.idxmax also have references to axis, should I remove those as well? I can update this PR with that change or open a separate PR for that fix if it's needed.

[rhshadrach]

I see Series.idxmin and Series.idxmax also have references to axis, should I remove those as well? I can update this PR with that change or open a separate PR for that fix if it's needed.

The axis argument in these methods will remain; the argument was only removed in the groupby variant.

[jordan-d-murphy]

@rhshadrach can you give another look at this when you get time? I've addressed the changes you requested

[rhshadrach]

lgtm

[rhshadrach]

Thanks @jordan-d-murphy!