Skip to content
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.

Sign up for GitHub

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

Jump to bottom

DOC Ensures that load_wine passes numpydoc #22469

Merged
merged 4 commits into from
Mar 21, 2022
Merged

DOC Ensures that load_wine passes numpydoc #22469

merged 4 commits into from
Mar 21, 2022

Conversation

sharmadharmpal
Copy link
Contributor

Reference Issues/PRs
Addresses #21350

What does this implement/fix? Explain your changes.
DOC Ensures that load_wine passes numpydoc validation

Any other comments?
No

Thank You

@sharmadharmpal
Description added for load_wine function and Source information has m…
25be32b 
…oved to the beginning of function documentation
jmloyola
jmloyola reviewed Feb 14, 2022
View reviewed changes
Copy link
Member

@jmloyola jmloyola left a comment

Choose a reason for hiding this comment

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

@sharmadharmpal Thanks for the PR :D

I left one comment with something to change.

Also, try to keep the length of the documentation lines to less than 80 characters. I think it is not a written rule, yet it is something that is applied in most of the project code. Maybe someone else can correct me on this.

Finally, it'd be great if we add items for each of the data return attributes. You can look to this PR for reference. You have to do something like:

"""
    Returns
    -------
    data : :class:`~sklearn.utils.Bunch`
        Dictionary-like object, with the following attributes:

        - data : {ndarray, dataframe} of shape (178, 13)
            The data matrix. If `as_frame=True`, `data` will be a pandas
            DataFrame.
"""

sklearn/datasets/_base.py Outdated
Comment on lines 470 to 472
A tuple of two ndarrays. The first contains a 2D array of shape (178, 13)
with each row representing one sample and each column representing the features.
The second array of shape (178,) contains the target samples.
Copy link
Member

Choose a reason for hiding this comment

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

This is not the case when as_frame=True where data and target are pandas objects.

Copy link
Member

Choose a reason for hiding this comment

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

This is a good point that we let passed in other PRs :)

So I would propose ndarray or dataframe or ndarray or series?

Copy link
Member

Choose a reason for hiding this comment

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

This is not the case when as_frame=True where data and target are pandas objects.

This PR is an example of the wording we used to take this point into account

@thomasjpfan
Copy link
Member

Also, try to keep the length of the documentation lines to less than 80 characters.

Since moving to the black, it's okay to use up to 88 now.

@sharmadharmpal
Copy link
Contributor Author

It was my first change in open source. It have copied from other's comments in the code and modified according to function definition, next time will try as recommended..

@jmloyola
Copy link
Member

It's Ok @sharmadharmpal.
You did a great job.
As the others said, you only need to fix the description of (data, target) for this PR to be merged.

@sharmadharmpal
Copy link
Contributor Author

It's Ok @sharmadharmpal. You did a great job. As the others said, you only need to fix the description of (data, target) for this PR to be merged.

Hi @jmloyola

can you please confirm below is correct? Or please guide me.

(data, target) : tuple if ``return_X_y`` is True
    A tuple of two ndarrays. The first contains a 2D array of shape (178, 13)
    with each row representing one sample and each column representing the features.
    The second array of shape (178,) contains the target samples.  If `as_frame=True`, both arrays are pandas objects,
    i.e. `X` a dataframe and `y` a series.

@jmloyola
Copy link
Member

It's fine. I'd only add in the beginning:

"A tuple of two ndarrays by default."

@thomasjpfan thomasjpfan mentioned this pull request Feb 22, 2022
Closed
@jeremiedbb jeremiedbb added the Quick Review For PRs that are quick to review label Mar 18, 2022
jeremiedbb
jeremiedbb approved these changes Mar 21, 2022
View reviewed changes
Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

Thanks @sharmadharmpal

@jeremiedbb jeremiedbb merged commit d49f7c3 into scikit-learn:main Mar 21, 2022
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Apr 6, 2022
@sharmadharmpal @jeremiedbb @glemaitre
DOC Ensures that load_wine passes numpydoc (scikit-learn#22469)
d2ebdf7 
Co-authored-by: Jérémie du Boisberranger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ArturoAmorQ ArturoAmorQ ArturoAmorQ left review comments

@glemaitre glemaitre glemaitre left review comments

@jmloyola jmloyola jmloyola left review comments

@jeremiedbb jeremiedbb jeremiedbb approved these changes

Assignees
No one assigned
Labels
Documentation module:datasets No Changelog Needed Quick Review For PRs that are quick to review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sharmadharmpal @thomasjpfan @jmloyola @glemaitre @jeremiedbb @ArturoAmorQ