Skip to content

DOC Ensures that load_wine passes numpydoc#22469

Merged
jeremiedbb merged 4 commits intoscikit-learn:mainfrom
sharmadharmpal:DocString_21350_load_wine
Mar 21, 2022
Merged

DOC Ensures that load_wine passes numpydoc#22469
jeremiedbb merged 4 commits intoscikit-learn:mainfrom
sharmadharmpal:DocString_21350_load_wine

Conversation

@sharmadharmpal
Copy link
Copy Markdown
Contributor

@sharmadharmpal sharmadharmpal commented Feb 13, 2022

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
Copy Markdown
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
Copy Markdown
Member

@jmloyola jmloyola Feb 13, 2022

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
Copy Markdown
Member

@glemaitre glemaitre Feb 14, 2022

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
Copy Markdown
Member

@ArturoAmorQ ArturoAmorQ Feb 15, 2022

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
Copy Markdown
Member

thomasjpfan commented Feb 14, 2022

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
Copy Markdown
Contributor Author

sharmadharmpal commented Feb 14, 2022

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
Copy Markdown
Member

jmloyola commented Feb 14, 2022

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
Copy Markdown
Contributor Author

sharmadharmpal commented Feb 16, 2022

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
Copy Markdown
Member

jmloyola commented Feb 16, 2022

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
Copy Markdown
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 <34657725+jeremiedbb@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@glemaitre glemaitre glemaitre left review comments

@ArturoAmorQ ArturoAmorQ ArturoAmorQ left review comments

@jeremiedbb jeremiedbb jeremiedbb approved these changes

+1 more reviewer

@jmloyola jmloyola jmloyola left review comments

Reviewers whose approvals may not affect merge requirements

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