-
-
Notifications
You must be signed in to change notification settings - Fork 25.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DOC added example to permutation importance #16460
DOC added example to permutation importance #16460
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
LogisticRegression(...) | ||
|
||
>>> result = permutation_importance(clf, X, y, n_repeats=10, | ||
... random_state=0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you align this line such that random_state
would be under clf
of the previous line
-------- | ||
>>> from sklearn.linear_model import LogisticRegression | ||
>>> from sklearn.inspection import permutation_importance | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
>>> from sklearn.linear_model import LogisticRegression | ||
>>> from sklearn.inspection import permutation_importance | ||
|
||
>>> X = [[1,9,9],[1,9,9],[1,9,9], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add spaces after each comma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apart of the last one :)
|
||
>>> 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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>>> y = [1,1,1,0,0,0] | |
>>> y = [1, 1, 1, 0, 0, 0] |
>>> from sklearn.linear_model import LogisticRegression | ||
>>> from sklearn.inspection import permutation_importance | ||
|
||
>>> X = [[1,9,9],[1,9,9],[1,9,9], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>>> X = [[1,9,9],[1,9,9],[1,9,9], | |
>>> X = [[1, 9, 9], [1, 9, 9], [1, 9, 9], |
>>> 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]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... [0,9,9],[0,9,9],[0,9,9]] | |
... [0, 9, 9], [0, 9, 9], [0, 9, 9]] |
array([0.5, 0. , 0. ]) | ||
|
||
>>> result.importances_std | ||
array([0.2236068, 0. , 0. ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't it weird that the number here is different from the one on the dictionary couple of line above?
'importances_std': array([0.16666667, 0. , 0. ]), | ||
'importances': array([[0.33333333, 0.66666667], | ||
[0. , 0. ], | ||
[0. , 0. ]])} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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! |
You have some linter issues (line too long). Related to linting, if you haven't, you may find pep8 useful. |
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/
|
@glemaitre Good morning, hmmm, now the checks from before are passing, but there new ones failing. |
I think this is the part of the error message relevant to this PR:
which means there's an issue with the whitespaces at the begining of the line you're changing/adding. |
... [0, 9, 9],[0, 9, 9],[0, 9, 9]] | ||
>>> y = [1, 1, 1, 0, 0, 0] | ||
>>> clf = LogisticRegression().fit(X, y) | ||
LogisticRegression(...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LogisticRegression(...) |
Tentative solution for the failed check... hope it helps...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cmarmo Hi Chiara, thanks a lot! All the best!
@glemaitre Hi Guillaume, @adrinjalali Hi Adrin, seems like the tests are passing! wow. cool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM, thanks @magda-zielinska
>>> result = permutation_importance(clf, X, y, n_repeats=10, | ||
... random_state=0) | ||
>>> result.importances_mean | ||
array([0.46666667, 0. , 0. ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use ellipsis to avoid precision issues on different platforms:
array([0.46666667, 0. , 0. ]) | |
array([0.4666..., 0. , 0. ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adrinjalali all right, I changed it! thanks. the local tests passed. pushing it here
>>> result.importances_mean | ||
array([0.46666667, 0. , 0. ]) | ||
>>> result.importances_std | ||
array([0.22110832, 0. , 0. ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well
array([0.22110832, 0. , 0. ]) | |
array([0.2211..., 0. , 0. ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Introduced suggestions made by @glemaitre to the example of permutation importance
#16223
hope it works out this time. looking foreword to feedback. all the best!
together with @fraboeni