Skip to content

Adding fastpath and name parameters to docstring for Series class and others. #27235

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 7 commits into from

Conversation

Adam-Klaum
Copy link
Contributor

@Adam-Klaum Adam-Klaum commented Jul 4, 2019

I modified the docstring for the Series class to include the "fastpath" and "name" parameters. I searched the code base, and also modified the docstrings for the following classes in a similar way:

Categorial
CategoricalIndex
Grouping
Index
MultiIndex
PeriodIndex
RangeIndex

It appears that fastpath is definitely used internally, and is not deprecated. There were no other examples of fastpath being documented in the parameters section of a docstring, so I went with this convention:

fastpath : bool, default False
Internal use only

@WillAyd WillAyd added the Docs label Jul 5, 2019
@WillAyd
Copy link
Member

WillAyd commented Jul 5, 2019

Thanks for the PR! I'm not sure about documenting this more. The note is pretty clear that it's internal only I think this generates more questions than it solves.

Would the alternate just to be to remove it from all documentation?

@Adam-Klaum
Copy link
Contributor Author

Will, happy to help!

I thought about removing it entirely, as that would probably be the clearest message to the user. However, it looks like Sphinx is using autosummary to build the first part of the documentation and is querying the class' init method to do so. So as long as fastpath is a valid parameter to the constructor, it will show in the documentation.

I'm new to Sphinx so I'm not sure if there is an easy way to override this behavior to hide certain parameters. So I think our options are:

  1. Do as I have done in the pull request to at least let users know to leave the fastpath parameter alone.

  2. Do some Sphinx wizardry so that it ignores the fastpath parameter when building the doc.

  3. Eliminate the fastpath methodology from the code base.

I think I understand why the fastpath functionality was originally used internally. There are way more sanity checks and exception handlers used when fastpath=False. fastpath=True assumes that the caller has done their homework and isn't passing in garbage. Makes sense for an internally used function.

I'm open to any of the three options, what do you think?

@jbrockmendel
Copy link
Member

I think I've identified the only place where Series fastpath is used and plan to get rid of it.

@Adam-Klaum
Copy link
Contributor Author

Adam-Klaum commented Jul 5, 2019

I only found one use of it for Series in this block of code in pandas/core/frame.py

    def _box_col_values(self, values, items):
        """
        Provide boxed values for a column.
        """
        klass = self._constructor_sliced
        return klass(values, index=self.index, name=items, fastpath=True)

@Adam-Klaum
Copy link
Contributor Author

It sounds like the direction is to purge fastpath from the code base. I'm going to go ahead and close this request in lieu of future pull requests that propose code changes to remove fastpath from classes other than Series.

@Adam-Klaum Adam-Klaum closed this Jul 9, 2019
@timhoffm
Copy link
Contributor

timhoffm commented Aug 16, 2019

As long as fastpath is not removed (and I don't see any immediate motion on that), it's better to document it as "Internal use only" than having people wonder what this is for and maybe even use it.

So I think this is still a valuable addition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DOC: Parameters name and fastpath of Series are not documented
4 participants