Skip to content

DOC Ensures that get_data_home passes numpydoc validation#22259

Merged
thomasjpfan merged 6 commits intoscikit-learn:mainfrom
purna135:numpydoc_get_data_home
Jan 24, 2022
Merged

DOC Ensures that get_data_home passes numpydoc validation#22259
thomasjpfan merged 6 commits intoscikit-learn:mainfrom
purna135:numpydoc_get_data_home

Conversation

@purna135
Copy link
Copy Markdown
Contributor

Reference Issues/PRs

Addresses #21350

What does this implement/fix? Explain your changes.

This PR fixes the following error that was appearing for numpydoc validation for sklearn.datasets._base.get_data_home

  • RT01: No Returns section found

Any other comments?

Thanks

@purna135 purna135 changed the title DOC Ensures that sklearn.datasets._base.get_data_home passes numpydoc validation DOC Ensures that get_data_home passes numpydoc validation Jan 21, 2022
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.

Thanks for the PR @purnachandramansingh. This LGTM!

I left a suggestion about the return description.

Also, I think it would be nice if we replace "dir" with "directory" in the docstrings.

Returns
-------
data_home: str
The path of the scikit-learn data dir.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
The path of the scikit-learn data dir.
Path of the scikit-learn data directory.

Copy link
Copy Markdown
Member

@jjerphan jjerphan Jan 21, 2022

Choose a reason for hiding this comment

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

Based on the other docstrings, I guess we can unintended the text a bit and keep it verbose, like what @jmloyola proposes.

Suggested change
The path of the scikit-learn data dir.
The path of the scikit-learn data directory.

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 after addressing the main thread.

Returns
-------
data_home: str
The path of the scikit-learn data dir.
Copy link
Copy Markdown
Member

@jjerphan jjerphan Jan 21, 2022

Choose a reason for hiding this comment

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

Based on the other docstrings, I guess we can unintended the text a bit and keep it verbose, like what @jmloyola proposes.

Suggested change
The path of the scikit-learn data dir.
The path of the scikit-learn data directory.

@purna135
Copy link
Copy Markdown
Contributor Author

I am confused about whether to do these changes or not.
finally, I am making a PR with these changes

Thanks for the PR @purnachandramansingh. This LGTM!

I left a suggestion about the return description.

Also, I think it would be nice if we replace "dir" with "directory" in the docstrings.

@purna135 purna135 requested review from jjerphan and jmloyola January 24, 2022 09:09
@jmloyola
Copy link
Copy Markdown
Member

LGTM

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. just one comment for consistency.

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
@purna135
Copy link
Copy Markdown
Contributor Author

Thank you so much @jjerphan

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.

LGTM

@purna135
Copy link
Copy Markdown
Contributor Author

Thanks @thomasjpfan

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants