DOC Ensures that LatentDirichletAllocation passes numpydoc validation by g4brielvs · Pull Request #20402 · scikit-learn/scikit-learn

g4brielvs · 2021-06-26T21:01:23Z

Reference Issues/PRs

Addresses #20308

What does this implement/fix? Explain your changes.

This PR ensures LatentDirichletAllocation is compatible with numpydocd

Remove LatentDirichletAllocation from DOCSTRING_IGNORE_LIST
Minor style fixes.

Any other comments?

Thanks #DataUmbrella

rth

Thanks @g4brielvs !

rth · 2021-06-27T06:20:31Z

sklearn/decomposition/_lda.py

        Returns
        -------
-        self
+        self:


Suggested change

self:

self

self : object for consistency with other places in the package

yes, but I don't think it's correct. The base class for all estimators is BaseEstimator not object, so I think there is no point in indicating the object dtype.

rth · 2021-06-27T06:20:40Z

sklearn/decomposition/_lda.py

        Returns
        -------
-        self
+        self:


Suggested change

self:

self

self : object

The initial version was correct for cases where there are no types, and it's a bit pointless to indicate object dtype for estimators which is also not accurate https://numpydoc.readthedocs.io/en/latest/format.html#parameters

cc @glemaitre

it's a bit pointless to indicate object dtype for estimators which is also not accurate

I agree. However, we have this way of documenting everywhere. I would prefer to keep this inaccurate convention until we do a find/replace regex in another PR. Would it not be easier to make the change?

Right, but my point is there is no issue with this line of docstring. It should pass the numpydoc validation, I think? And if so I would rather we didn't change it instead of introducing unnecessary code churn with a solution we know is not helpful / correct.

self : object is used elsewhere, but 27% of estimators still use the current conversion,

$ rg "^\s+self$" | grep self | wc -l 38 $ rg "^\s+self\s*:$" | grep self | wc -l 2 $ rg "^\s+self\s*:\s*object$" | grep self | wc -l 103

@rth Thank you so much for reviewing my PR. The numpydoc validation requires a description and that's the reason a added a tautology. I'll be more than happy to make any changes according to your guidance.

I think the description is ok. The point of @rth is about self : object vs self alone on the current line, not the description below.

rth · 2021-06-27T06:23:56Z

sklearn/decomposition/_lda.py

+    [2] "Stochastic Variational Inference", Matthew D. Hoffman, David M. Blei,
+        Chong Wang, John Paisley, 2013
+
+    [3] Matthew D. Hoffman's onlineldavb code. Link:


I think,

Suggested change

[2] "Stochastic Variational Inference", Matthew D. Hoffman, David M. Blei,

Chong Wang, John Paisley, 2013

[3] Matthew D. Hoffman's onlineldavb code. Link:

.. [2] "Stochastic Variational Inference", Matthew D. Hoffman, David M. Blei,

Chong Wang, John Paisley, 2013

.. [3] Matthew D. Hoffman's onlineldavb code. Link:

but need to check the rendering (ci/circleci: doc artifact CI job), currently it doesn't look ideal: https://142780-843222-gh.circle-artifacts.com/0/doc/modules/generated/sklearn.decomposition.LatentDirichletAllocation.html

It might raise an error if the article is not linked (somewhere we expect [2]_) in the docstring. If this is not the case, we could either link where it is meaningful, otherwise, we could as well remove the reference.

reshamas · 2021-06-27T14:16:09Z

Updating with correct spelling: #DataUmbrella sprint

glemaitre · 2021-06-28T09:54:51Z

OK.

…

On Mon, 28 Jun 2021 at 11:44, Roman Yurchak ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In sklearn/decomposition/_lda.py <#20402 (comment)> : > Returns ------- - self + self: Right, but my point is there is no issue with this line of docstring. It should pass the numpydoc validation, I think? And if so I would rather we didn't change it instead of introducing unnecessary code churn with a solution we know is not helpful / correct. self : object is used elsewhere, but 27% of estimators use the current conversion, $ rg "^\s+self$" | grep self | wc -l 38 $ rg "^\s+self\s*:$" | grep self | wc -l 2 $ rg "^\s+self\s*:\s*object$" | grep self | wc -l 103 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20402 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABY32P3LWJK22RLIEIMMEW3TVBAAXANCNFSM47LWDQNA> .

-- Guillaume Lemaitre Scikit-learn @ Inria Foundation https://glemaitre.github.io/

rth

The numpydoc validation requires a description and that's the reason a added a tautology.

If untyped self doesn't pass validation than it's an instance of numpy/numpydoc#242. LGTM for the rest except for the https://github.com/scikit-learn/scikit-learn/pull/20402/files#r659270234 for which I'm still not sure.

glemaitre · 2021-07-20T17:48:05Z

I made the changes in another PR and merge with the current branch and authorship.
Thanks @g4brielvs

DOC Ensures that LatentDirichletAllocation passes numpydoc validation

88d339f

github-actions bot added module:decomposition Documentation labels Jun 26, 2021

rth reviewed Jun 27, 2021

View reviewed changes

thomasjpfan mentioned this pull request Jun 27, 2021

Ensure that docstrings pass numpydoc validation #20308

Closed

rth approved these changes Jul 6, 2021

View reviewed changes

glemaitre self-requested a review July 7, 2021 10:07

glemaitre assigned glemaitre and unassigned glemaitre Jul 7, 2021

glemaitre requested review from glemaitre and removed request for glemaitre July 7, 2021 10:14

glemaitre self-assigned this Jul 20, 2021

glemaitre removed their request for review July 20, 2021 16:30

glemaitre mentioned this pull request Jul 20, 2021

DOC Ensures that LatentDirichletAllocation passes numpydoc validation #20574

Merged

glemaitre closed this in #20574 Jul 20, 2021

Uh oh!

Conversation

g4brielvs commented Jun 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

rth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rth Jun 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rth Jun 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reshamas commented Jun 27, 2021

Uh oh!

glemaitre commented Jun 28, 2021 via email

Uh oh!

rth left a comment

Choose a reason for hiding this comment

Uh oh!

glemaitre commented Jul 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

g4brielvs commented Jun 26, 2021 •

edited

Loading

rth Jun 28, 2021 •

edited

Loading

rth Jun 28, 2021 •

edited

Loading