Skip to content

Conversation

@emassoulie
Copy link
Contributor

Closes #1791 .

In order to give proper documentation to the Selector class's expand and expand_index method, added a doc entry to the class itself (which was already public) and references to that documentation in the docstring of each of its inherited classes.

Copy link
Member

@rcap107 rcap107 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @emassoulie! I added some comments on the format of the docs.

I think the user guide should also be updated to explain the use of expand and expand_index.


def all():
"""Select all columns.
"""Select all columns. See also :class:`skrub.selectors.Selector()`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure anymore there should be a "See also" referring to Selector for all these functions, especially since we are saying that the Selector should not be used by the users.

However, we might want to use the See Also field to refer to the user guide, where we explain that the function is returning a selector and that the selector has expand and expand_index.

This also means that we need to add a proper mention of this in the user guide, with examples and explanation.

Also note the formatting of the See Also: it's a specific field in the numpy docs. It's used in various places in the library, like the StringEncoder.

Parameters
----------
df : dataframe
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be an example here showing what expand does when applied to a dataframe.

def expand(self, df):
"""Lists the column names that the selector would retain if applied to
the dataframe `df`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either here or in the examples there should be a message mentioning that this can be useful for using the selectors with standard dataframe libraries.

Comment on lines +245 to +247
The list of `df`'s columns that the item would select. In effect,
running `df[sel.expand(df)]` should give the exact same result as
`sel.transform(df)`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lines I removed here are a better fit for the Notes section, or for an example.

Suggested change
The list of `df`'s columns that the item would select. In effect,
running `df[sel.expand(df)]` should give the exact same result as
`sel.transform(df)`.
The list of `df`'s columns that the item would select.

def expand_index(self, df):
"""Lists the indices of dataframe `df`'s columns that the selector
would retain if applied to `df`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments I made for the expand case also apply here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DOC - Improve the documentation of the selector functions

2 participants