Skip to content

DOC Ensures that fetch_rcv1 passes numpydoc validation#22225

Merged
thomasjpfan merged 4 commits intoscikit-learn:mainfrom
SamAdamDay:numpydoc_datasets._rcv1.fetch_rcv1
Jan 23, 2022
Merged

DOC Ensures that fetch_rcv1 passes numpydoc validation#22225
thomasjpfan merged 4 commits intoscikit-learn:mainfrom
SamAdamDay:numpydoc_datasets._rcv1.fetch_rcv1

Conversation

@SamAdamDay
Copy link
Copy Markdown
Contributor

@SamAdamDay SamAdamDay commented Jan 15, 2022

Reference Issues/PRs

Addresses #21350

What does this implement/fix? Explain your changes.

Ensured that datasets._rcv1.fetch_rcv1 passes numpydoc validation.

  • Removed the function from the list FUNCTION_DOCSTRING_IGNORE_LIST.
  • Made sure that the second return value has a description.
  • Added sentence to each return value stating under which condition it is returned.
  • Changed the type description for (data, target) to tuple from tuple if ``return_X_y`` is True. It doesn't seem that the type of this return value depends on the value of return_X_y. Instead, whether or not such a return value exists depends on this.

Any other comments?

The type description tuple if ``return_X_y`` is True appears in several other function docstrings.

thomasjpfan
thomasjpfan approved these changes Jan 15, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @SparklePigBang !

LGTM!

@thomasjpfan thomasjpfan added the Quick Review For PRs that are quick to review label Jan 15, 2022
@thomasjpfan thomasjpfan mentioned this pull request Jan 17, 2022
Closed
jjerphan
jjerphan approved these changes Jan 18, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM.

@jjerphan jjerphan removed the Quick Review For PRs that are quick to review label Jan 18, 2022
@jmloyola
Copy link
Copy Markdown
Member

jmloyola commented Jan 21, 2022

Thanks for the PR @SparklePigBang 😃
LGTM!

I left a comment with something that I believe would improve the documentation. Let me know what you think.

@SamAdamDay @jjerphan
DOC Tweak to datasets._rcv1.fetch_rcv1 docstring
9e6bcef 
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
@SamAdamDay
Copy link
Copy Markdown
Contributor Author

SamAdamDay commented Jan 21, 2022

I left a comment with something that I believe would improve the documentation. Let me know what you think.

Thank you for the suggestion. It looks good to me. My only concern is that I haven't been able to find another docstring where the return attributes are itemised in this way. Would this be a problem?

@jmloyola
Copy link
Copy Markdown
Member

jmloyola commented Jan 21, 2022

That's true, I think there is no other returned dictionary documented this way.

What do you think @thomasjpfan @jjerphan?
Should we leave as it is or should we add the items? I think it looks better with the items, but we may need to adapt other returns dictionary (or Bunchs) to mantain consistency in the future.

jmloyola
jmloyola reviewed Jan 21, 2022
View reviewed changes
sklearn/datasets/_rcv1.py Outdated
dataset : :class:`~sklearn.utils.Bunch`
Dictionary-like object. Returned only if `return_X_y` is False. Has
the following attributes.
Dictionary-like object. Returned only if `return_X_y` is False.
Copy link
Copy Markdown
Member

@jmloyola jmloyola Jan 21, 2022

Choose a reason for hiding this comment

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

There is a failing test related to black because there is a trailing white space in this line.

@jjerphan
Copy link
Copy Markdown
Member

jjerphan commented Jan 21, 2022

That's true, I think there is no other returned dictionary documented this way.

What do you think @thomasjpfan @jjerphan? Should we leave as it is or should we add the items? I think it looks better with the items, but we may need to adapt other returns dictionary (or Bunchs) to mantain consistency in the future.

This looks better to me with this new comment. Let's merge after having the linting fixed.

@thomasjpfan thomasjpfan changed the title DOC Ensures that datasets._rcv1.fetch_rcv1 passes numpydoc validation DOC Ensures that fetch_rcv1 passes numpydoc validation Jan 23, 2022
@thomasjpfan thomasjpfan merged commit cdf0d69 into scikit-learn:main Jan 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@jjerphan jjerphan jjerphan 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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@SamAdamDay @jmloyola @jjerphan @thomasjpfan