DOC add missing attributes to LocalOutlierFactor by j2heng · Pull Request #17696 · scikit-learn/scikit-learn

j2heng · 2020-06-24T10:40:44Z

Reference Issues/PRs

References #14312

What does this implement/fix? Explain your changes.

Added effective_metric_ , effective_metric_params_ and n_samples_fit_ to docstring in LocalOutlierFactor.
LocalOutlierFactor removed from the list IGNORED in test_docstring_parameters.py.

lesteve · 2020-06-24T12:15:30Z

To fix the linting issues:
the build log shows that you have flake8 errors (one trailing white space, and missing whitespace before a comma):

sklearn/tests/test_docstring_parameters.py:237:21: E231 missing whitespace after ','
               'MDS','MiniBatchKMeans', 
                    ^
sklearn/tests/test_docstring_parameters.py:237:40: W291 trailing whitespace
               'MDS','MiniBatchKMeans', 
                                       ^

I advise you to invest some time to configure on-the-fly flake8 checks inside your editor of choice. This allows you to spot these kind of problems while editing. There may be some materials about this in the Developer doc (not sure).

lesteve

Some comments (not 100% sure about them) ...

lesteve · 2020-06-24T13:10:58Z

sklearn/neighbors/_lof.py


+    effective_metric_ : str
+        The effective metric used for the distance computation.
+        If `metric` is 'minkowski', `effective_metric_` is equivalent to


I am not sure you want all these details. It is quite hard to keep in sync when the code changes. Maybe this is enough (not really sure)?

Effective metric used for the distance computation.

I agree with Loic. The details should be given in the Parameters. Once it has been fitted, we just want to know that this the metric used.

lesteve · 2020-06-24T13:11:34Z

sklearn/neighbors/_lof.py

+
+    effective_metric_params_ : dict
+        The effective additional keyword arguments for the metric function.
+        This is equivalent to `metric_params` in parameters if metric_params


Same thing here maybe this is enough?

Additional keyword arguments for the metric function.

I also agree with @lesteve

glemaitre

An additional comment to @lesteve review.

glemaitre · 2020-06-24T16:42:45Z

sklearn/neighbors/_lof.py


+    effective_metric_ : str
+        The effective metric used for the distance computation.
+        If `metric` is 'minkowski', `effective_metric_` is equivalent to


I agree with Loic. The details should be given in the Parameters. Once it has been fitted, we just want to know that this the metric used.

glemaitre · 2020-06-24T16:43:42Z

sklearn/neighbors/_lof.py

+    n_samples_fit_ : int
+        It is the number of samples in the fitted data.
+
        .. versionadded:: 0.20


This version added should not be here. I assume it was belonging to the section of the parameter offset_.

glemaitre · 2020-06-24T16:43:51Z

sklearn/neighbors/_lof.py

+
+    effective_metric_params_ : dict
+        The effective additional keyword arguments for the metric function.
+        This is equivalent to `metric_params` in parameters if metric_params


I also agree with @lesteve

j2heng · 2020-06-24T22:39:55Z

Thank you all for your reviews.
I made a new commit simplifying the attribute descriptions.

cmarmo · 2020-06-25T08:01:06Z

@glemaitre, @lesteve, have comments been addressed? Thanks!

ogrisel

Some more minor comments, other than that LGTM.

ogrisel · 2020-06-25T08:11:44Z

sklearn/neighbors/_lof.py

+    n_samples_fit_ : int
+        It is the number of samples in the fitted data.
+
+
+
    Examples


In the docstring of other classes in the code base we do not insert extra blank lines between sections. Let's be consistent :

Suggested change

n_samples_fit_ : int

It is the number of samples in the fitted data.

Examples

n_samples_fit_ : int

It is the number of samples in the fitted data.

Examples

ogrisel · 2020-06-25T08:13:44Z

sklearn/tests/test_docstring_parameters.py

               'OrthogonalMatchingPursuit',
               'PLSCanonical', 'PLSSVD',
               'PassiveAggressiveClassifier'}
-


Let's not edit lines that are unrelated to the change at hand. Our coding conventions tolerate this blank line, so let's keep it.

glemaitre · 2020-06-25T08:17:04Z

You will need to merge master into your branch and solve the conflict (it should be in the list of estimator that should be bypass).

lesteve · 2020-06-25T08:58:39Z

You will need to merge master into your branch and solve the conflict (it should be in the list of estimator that should be bypass).

I resolved the conflicts via the web UI. This works well enough IMO for simple conflicts.

ogrisel · 2020-06-25T12:52:21Z

@j2heng you will have to git pull --rebase origin branch_missing_attr_1 to pull the merge commit to your local clone before being able to push the commits to address the remaining comments.

j2heng · 2020-06-25T16:09:07Z

I'm on it

ogrisel · 2020-06-26T07:20:02Z

@j2heng you need to remove LocalOutlierFactor from the list of ignored class in the test that checks the attributes in docstrings.

glemaitre · 2020-06-26T07:48:50Z

Uhm I might done this when handling the merge to master

glemaitre · 2020-06-26T07:51:34Z

@j2heng sorry about that this is good to be merged. Thanks

Co-authored-by: Loïc Estève <loic.esteve@ymail.com> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

j2heng added 2 commits June 24, 2020 12:22

DOC add missing attributes to LocalOutlierFactor

9955c9b

fix test_docstring_parameters IGNORE

0fce2b3

github-actions bot added the module:neighbors label Jun 24, 2020

fix linting errors

765ef71

lesteve reviewed Jun 24, 2020

View reviewed changes

glemaitre reviewed Jun 24, 2020

View reviewed changes

simplified descriptions

3b87444

ogrisel reviewed Jun 25, 2020

View reviewed changes

Merge branch 'master' into branch_missing_attr_1

d266198

j2heng and others added 5 commits June 25, 2020 20:26

deleted extra blank lines

a71f53b

resolve merge conflit

6548cad

Update test_docstring_parameters.py

31f374f

Merge branch 'master' into branch_missing_attr_1

dc46f2d

Update test_docstring_parameters.py

380ae1b

glemaitre assigned glemaitre and unassigned glemaitre Jun 26, 2020

glemaitre self-assigned this Jun 26, 2020

remove lof from the list

2f457b9

glemaitre merged commit 3491e07 into scikit-learn:master Jun 26, 2020

j2heng deleted the branch_missing_attr_1 branch June 26, 2020 12:10

glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Jul 17, 2020

DOC add missing attributes to LocalOutlierFactor (scikit-learn#17696)

3b708aa

Co-authored-by: Loïc Estève <loic.esteve@ymail.com> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020

DOC add missing attributes to LocalOutlierFactor (scikit-learn#17696)

f78695f

Co-authored-by: Loïc Estève <loic.esteve@ymail.com> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Uh oh!

Conversation

j2heng commented Jun 24, 2020

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Uh oh!

lesteve commented Jun 24, 2020

Uh oh!

lesteve left a comment

Choose a reason for hiding this comment

Uh oh!

lesteve Jun 24, 2020 • edited by glemaitre Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glemaitre Jun 24, 2020

Choose a reason for hiding this comment

Uh oh!

lesteve Jun 24, 2020 • edited by glemaitre Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glemaitre Jun 24, 2020

Choose a reason for hiding this comment

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

glemaitre Jun 24, 2020

Choose a reason for hiding this comment

Uh oh!

glemaitre Jun 24, 2020

Choose a reason for hiding this comment

Uh oh!

glemaitre Jun 24, 2020

Choose a reason for hiding this comment

Uh oh!

j2heng commented Jun 24, 2020

Uh oh!

cmarmo commented Jun 25, 2020

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

ogrisel Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

ogrisel Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

glemaitre commented Jun 25, 2020

Uh oh!

lesteve commented Jun 25, 2020

Uh oh!

ogrisel commented Jun 25, 2020

Uh oh!

j2heng commented Jun 25, 2020

Uh oh!

ogrisel commented Jun 26, 2020

Uh oh!

glemaitre commented Jun 26, 2020

Uh oh!

glemaitre commented Jun 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lesteve Jun 24, 2020 •

edited by glemaitre

Loading

lesteve Jun 24, 2020 •

edited by glemaitre

Loading