DOC added example to permutation importance by magda-zielinska · Pull Request #16460 · scikit-learn/scikit-learn

magda-zielinska · 2020-02-17T14:23:11Z

Introduced suggestions made by @glemaitre to the example of permutation importance

hope it works out this time. looking foreword to feedback. all the best!

together with @fraboeni

glemaitre

The failure that you have is because of the need to synchronize your branch with the upstream/master branch. You can merge upstream/master into your branch and it should be fixed.

glemaitre · 2020-02-17T18:23:21Z

sklearn/inspection/_permutation_importance.py

+    LogisticRegression(...)
+
+    >>> result = permutation_importance(clf, X, y, n_repeats=10,
+    ... random_state=0)


Could you align this line such that random_state would be under clf of the previous line

glemaitre · 2020-02-17T18:24:35Z

sklearn/inspection/_permutation_importance.py

+    --------
+    >>> from sklearn.linear_model import LogisticRegression
+    >>> from sklearn.inspection import permutation_importance
+


If you can remove all blank lines between each >>>. They will create some blocks on the html page (cf. https://93833-843222-gh.circle-artifacts.com/0/doc/modules/generated/sklearn.inspection.permutation_importance.html)

glemaitre · 2020-02-17T18:25:03Z

sklearn/inspection/_permutation_importance.py

+    >>> from sklearn.linear_model import LogisticRegression
+    >>> from sklearn.inspection import permutation_importance
+
+    >>> X = [[1,9,9],[1,9,9],[1,9,9],


add spaces after each comma

apart of the last one :)

glemaitre · 2020-02-17T18:25:21Z

sklearn/inspection/_permutation_importance.py

+
+    >>> X = [[1,9,9],[1,9,9],[1,9,9],
+    ...      [0,9,9],[0,9,9],[0,9,9]]
+    >>> y = [1,1,1,0,0,0]


Suggested change

>>> y = [1,1,1,0,0,0]

>>> y = [1, 1, 1, 0, 0, 0]

glemaitre · 2020-02-17T18:25:53Z

sklearn/inspection/_permutation_importance.py

+    >>> from sklearn.linear_model import LogisticRegression
+    >>> from sklearn.inspection import permutation_importance
+
+    >>> X = [[1,9,9],[1,9,9],[1,9,9],


Suggested change

>>> X = [[1,9,9],[1,9,9],[1,9,9],

>>> X = [[1, 9, 9], [1, 9, 9], [1, 9, 9],

glemaitre · 2020-02-17T18:26:05Z

sklearn/inspection/_permutation_importance.py

+    >>> from sklearn.inspection import permutation_importance
+
+    >>> X = [[1,9,9],[1,9,9],[1,9,9],
+    ...      [0,9,9],[0,9,9],[0,9,9]]


Suggested change

... [0,9,9],[0,9,9],[0,9,9]]

... [0, 9, 9], [0, 9, 9], [0, 9, 9]]

glemaitre · 2020-02-17T18:27:30Z

sklearn/inspection/_permutation_importance.py

+    array([0.5, 0. , 0. ])
+
+    >>> result.importances_std
+    array([0.2236068, 0.       , 0.       ])


isn't it weird that the number here is different from the one on the dictionary couple of line above?

ogrisel · 2020-02-20T17:33:09Z

sklearn/inspection/_permutation_importance.py

+    'importances_std': array([0.16666667, 0.        , 0.        ]),
+    'importances': array([[0.33333333, 0.66666667],
+        [0.        , 0.        ],
+        [0.        , 0.        ]])}


The full dictionary output should not be displayed as the results are stored in the result variable. I think showing the values of result.importances_mean and result.importances_std is enough.

magda-zielinska · 2020-02-25T17:45:54Z

@glemaitre dear Guillaume, i did the changes that you and @ogrisel Olivier suggested. Thanks a lot for your comments!

Managed to synchronize my branch with the help of @adrinjalali, big thanks for that!

…on_importance

adrinjalali · 2020-02-26T10:16:14Z

You have some linter issues (line too long). Related to linting, if you haven't, you may find pep8 useful.

glemaitre · 2020-02-26T10:31:07Z

And you have the package https://pypi.org/project/pycodestyle/ which automatically checks for these errors. Alternatively, we have a bash file in `build_tools/circle/linting.sh` (the same one which is failing in the CI) which you could execute locally using `bash build_tools/circle/linting.sh`. I allow getting the same errors than on the CI. Note that executing bash on Linux is straightforward while you will need a bit more effort in Windows.

…

On Wed, 26 Feb 2020 at 11:16, Adrin Jalali ***@***.***> wrote: You have some linter issues (line too long). Related to linting, if you haven't, you may find pep8 <https://www.python.org/dev/peps/pep-0008/> usefule. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16460?email_source=notifications&email_token=ABY32P4MYENOBGEQD6QJ5JDREY6PBA5CNFSM4KWSU23KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEM7UHYY#issuecomment-591348707>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABY32P74ZGSDXWDPVY5QAVDREY6PBANCNFSM4KWSU23A> .

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

magda-zielinska · 2020-02-26T10:56:39Z

@glemaitre Good morning, hmmm, now the checks from before are passing, but there new ones failing.
The logs don't tell me much. Could you help? All the best!

adrinjalali · 2020-02-26T13:38:08Z

I think this is the part of the error message relevant to this PR:

____________ ERROR collecting inspection/_permutation_importance.py ____________
/usr/share/miniconda/envs/testvenv/lib/python3.8/doctest.py:932: in find
    self._find(tests, obj, name, module, source_lines, globs, {})
/usr/share/miniconda/envs/testvenv/lib/python3.8/site-packages/_pytest/doctest.py:459: in _find
    doctest.DocTestFinder._find(  # type: ignore
/usr/share/miniconda/envs/testvenv/lib/python3.8/doctest.py:994: in _find
    self._find(tests, val, valname, module, source_lines,
/usr/share/miniconda/envs/testvenv/lib/python3.8/site-packages/_pytest/doctest.py:459: in _find
    doctest.DocTestFinder._find(  # type: ignore
/usr/share/miniconda/envs/testvenv/lib/python3.8/doctest.py:982: in _find
    test = self._get_test(obj, name, module, globs, source_lines)
/usr/share/miniconda/envs/testvenv/lib/python3.8/doctest.py:1066: in _get_test
    return self._parser.get_doctest(docstring, globs, name,
/usr/share/miniconda/envs/testvenv/lib/python3.8/doctest.py:668: in get_doctest
    return DocTest(self.get_examples(string, name), globs,
/usr/share/miniconda/envs/testvenv/lib/python3.8/doctest.py:682: in get_examples
    return [x for x in self.parse(string, name)
/usr/share/miniconda/envs/testvenv/lib/python3.8/doctest.py:644: in parse
    self._parse_example(m, name, lineno)
/usr/share/miniconda/envs/testvenv/lib/python3.8/doctest.py:703: in _parse_example
    self._check_prefix(source_lines[1:], ' '*indent + '.', name, lineno)
/usr/share/miniconda/envs/testvenv/lib/python3.8/doctest.py:798: in _check_prefix
    raise ValueError('line %r of the docstring for %s has '
E   ValueError: line 72 of the docstring for sklearn.inspection._permutation_importance.permutation_importance has inconsistent leading whitespace: '                                    ... random_state=0)'
____________ ERROR collecting inspection/_permutation_importance.py ____________
/usr/share/miniconda/envs/testvenv/lib/python3.8/doctest.py:932: in find
    self._find(tests, obj, name, module, source_lines, globs, {})
/usr/share/miniconda/envs/testvenv/lib/python3.8/site-packages/_pytest/doctest.py:459: in _find
    doctest.DocTestFinder._find(  # type: ignore
/usr/share/miniconda/envs/testvenv/lib/python3.8/doctest.py:994: in _find
    self._find(tests, val, valname, module, source_lines,
/usr/share/miniconda/envs/testvenv/lib/python3.8/site-packages/_pytest/doctest.py:459: in _find
    doctest.DocTestFinder._find(  # type: ignore
/usr/share/miniconda/envs/testvenv/lib/python3.8/doctest.py:982: in _find
    test = self._get_test(obj, name, module, globs, source_lines)
/usr/share/miniconda/envs/testvenv/lib/python3.8/doctest.py:1066: in _get_test
    return self._parser.get_doctest(docstring, globs, name,
/usr/share/miniconda/envs/testvenv/lib/python3.8/doctest.py:668: in get_doctest
    return DocTest(self.get_examples(string, name), globs,
/usr/share/miniconda/envs/testvenv/lib/python3.8/doctest.py:682: in get_examples
    return [x for x in self.parse(string, name)
/usr/share/miniconda/envs/testvenv/lib/python3.8/doctest.py:644: in parse
    self._parse_example(m, name, lineno)
/usr/share/miniconda/envs/testvenv/lib/python3.8/doctest.py:703: in _parse_example
    self._check_prefix(source_lines[1:], ' '*indent + '.', name, lineno)
/usr/share/miniconda/envs/testvenv/lib/python3.8/doctest.py:798: in _check_prefix
    raise ValueError('line %r of the docstring for %s has '
E   ValueError: line 72 of the docstring for sklearn.inspection._permutation_importance.permutation_importance has inconsistent leading whitespace: '                                    ... random_state=0)'
--------- generated xml file: /home/vsts/work/tmp_folder/test-data.xml ---------

which means there's an issue with the whitespaces at the begining of the line you're changing/adding.

…on_importance

cmarmo · 2020-03-13T10:49:48Z

sklearn/inspection/_permutation_importance.py

+    ...      [0, 9, 9],[0, 9, 9],[0, 9, 9]]
+    >>> y = [1, 1, 1, 0, 0, 0]
+    >>> clf = LogisticRegression().fit(X, y)
+    LogisticRegression(...)


Suggested change

LogisticRegression(...)

Tentative solution for the failed check... hope it helps...

@cmarmo Hi Chiara, thanks a lot! All the best!

…on_importance

magda-zielinska · 2020-04-08T11:46:36Z

@glemaitre Hi Guillaume, @adrinjalali Hi Adrin, seems like the tests are passing! wow. cool.
Thanks a lot for your lots of suggestions and help. xxx Magda

adrinjalali

Otherwise LGTM, thanks @magda-zielinska

adrinjalali · 2020-04-09T09:16:25Z

sklearn/inspection/_permutation_importance.py

+    >>> result = permutation_importance(clf, X, y, n_repeats=10,
+    ...                                 random_state=0)
+    >>> result.importances_mean
+    array([0.46666667, 0.        , 0.        ])


we use ellipsis to avoid precision issues on different platforms:

Suggested change

array([0.46666667, 0. , 0. ])

array([0.4666..., 0. , 0. ])

@adrinjalali all right, I changed it! thanks. the local tests passed. pushing it here

adrinjalali · 2020-04-09T09:16:46Z

sklearn/inspection/_permutation_importance.py

+    >>> result.importances_mean
+    array([0.46666667, 0.        , 0.        ])
+    >>> result.importances_std
+    array([0.22110832, 0.        , 0.        ])


Here as well

Suggested change

array([0.22110832, 0. , 0. ])

array([0.2211..., 0. , 0. ])

thomasjpfan

LGTM

new commit with introduced suggestions from the reviewer

0dff3a7

glemaitre reviewed Feb 17, 2020

View reviewed changes

ogrisel reviewed Feb 20, 2020

View reviewed changes

done changes suggested by the reviewers

88069f5

magda-zielinska added 2 commits February 25, 2020 18:53

Merge remote-tracking branch 'upstream/master' into example_permutati…

ba629d7

…on_importance

resolved the issue with whitespace

42f3c89

dealt with line lenght error

ec436de

magda-zielinska added 2 commits February 26, 2020 15:02

Merge remote-tracking branch 'upstream/master' into example_permutati…

2fc2dd0

…on_importance

changed the whitespaces

781692b

github-actions bot added the module:inspection label Mar 2, 2020

cmarmo reviewed Mar 13, 2020

View reviewed changes

magda-zielinska added 2 commits April 8, 2020 10:52

Merge remote-tracking branch 'upstream/master' into example_permutati…

8f5c6c9

…on_importance

fixing the test failures

8b2c97f

cmarmo added the Waiting for Reviewer label Apr 8, 2020

adrinjalali reviewed Apr 9, 2020

View reviewed changes

used elipsis to avoid platform conflicts in the precision

8bdcc50

adrinjalali approved these changes Apr 9, 2020

View reviewed changes

thomasjpfan approved these changes Apr 10, 2020

View reviewed changes

thomasjpfan changed the title ~~[MRG] DOC added example to permutation importance~~ DOC added example to permutation importance Apr 10, 2020

thomasjpfan merged commit 0447dd9 into scikit-learn:master Apr 10, 2020

cmarmo removed the Waiting for Reviewer label Apr 10, 2020

gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020

DOC added example to permutation importance (scikit-learn#16460)

87a0469

	>>> X = [[1,9,9],[1,9,9],[1,9,9],
	>>> X = [[1, 9, 9], [1, 9, 9], [1, 9, 9],

	... [0,9,9],[0,9,9],[0,9,9]]
	... [0, 9, 9], [0, 9, 9], [0, 9, 9]]

Uh oh!

Conversation

magda-zielinska commented Feb 17, 2020

Uh oh!

glemaitre 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!

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!

magda-zielinska commented Feb 25, 2020

Uh oh!

adrinjalali commented Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glemaitre commented Feb 26, 2020 via email

Uh oh!

magda-zielinska commented Feb 26, 2020

Uh oh!

adrinjalali commented Feb 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magda-zielinska commented Apr 8, 2020

Uh oh!

adrinjalali 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!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

adrinjalali commented Feb 26, 2020 •

edited

Loading