Skip to content

DOC Ensures that shrunk_covariance passes numpydoc validation#22260

Merged
glemaitre merged 9 commits intoscikit-learn:mainfrom
purna135:numpydoc_shrunk_covariance
Jan 27, 2022
Merged

DOC Ensures that shrunk_covariance passes numpydoc validation#22260
glemaitre merged 9 commits intoscikit-learn:mainfrom
purna135:numpydoc_shrunk_covariance

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 errors that were appearing for numpydoc validation for sklearn.covariance._shrunk_covariance.shrunk_covariance

  • SS03: Summary does not end with a period
  • SS05: Summary must start with infinitive verb, not third person (e.g. use "Generate" instead of "Generates")
  • PR09: Parameter "emp_cov" description should finish with "."

Any other comments?

Thanks

@jmloyola
Copy link
Copy Markdown
Member

Thanks @purnachandramansingh.
LGTM!

If you want to improve the documentation a little bit more (in my opinion ^^), we can change this too:

"""
    Notes
    -----
    The regularized (shrunk) covariance is given by::

        (1 - shrinkage) * cov + shrinkage * mu * np.identity(n_features)

    where `mu = trace(cov) / n_features`.
"""

These changes will make the documentation look like this:
imagen

@purna135
Copy link
Copy Markdown
Contributor Author

Thanks for your suggestions @jmloyola.
Let me do the necessary changes and push it again.

@jmloyola
Copy link
Copy Markdown
Member

Great!. Now, let's wait for more reviewers.
If everything is OK, they can merge this. 😃

@purna135
Copy link
Copy Markdown
Contributor Author

purna135 commented Jan 21, 2022

These changes will make the documentation look like this:
imagen

Thanks, @jmloyola I just wanted to know how did you generate this document as shown in the above image.

@jmloyola
Copy link
Copy Markdown
Member

I built the documentation following the Contributing page

@purna135
Copy link
Copy Markdown
Contributor Author

I built the documentation following the Contributing page

Thanks, @jmloyola, I will follow the same.

@purna135
Copy link
Copy Markdown
Contributor Author

purna135 commented Jan 24, 2022

Hi @jmloyola, could you please guide me on how to resolve these conflicts.

@jmloyola
Copy link
Copy Markdown
Member

You probably have to merge the new commits done to the main branch.

@purna135
Copy link
Copy Markdown
Contributor Author

ok got it, Thanks

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.

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.

Thanks for the PR @purnachandramansingh !

Can you remove the line here:

"sklearn.covariance._shrunk_covariance.shrunk_covariance",

to make sure the test pass?

(Also you may need to sync with main to make sure the test runs correctly)

@purna135
Copy link
Copy Markdown
Contributor Author

my bad!!, initially I removed this line but I think during my last sync It get replaced.
Let me check and commit again.

@purna135
Copy link
Copy Markdown
Contributor Author

Thanks for the PR @purnachandramansingh !

Can you remove the line here:

"sklearn.covariance._shrunk_covariance.shrunk_covariance",

to make sure the test pass?

(Also you may need to sync with main to make sure the test runs correctly)

done @thomasjpfan, please have a look.

Copy link
Copy Markdown
Member

@ArturoAmorQ ArturoAmorQ left a comment

Choose a reason for hiding this comment

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

LGTM

@glemaitre glemaitre merged commit 87e7d97 into scikit-learn:main Jan 27, 2022
@glemaitre
Copy link
Copy Markdown
Member

LGTM

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.

6 participants