Skip to content

[MRG] DOC add an example for permutation-importance#16223

Closed
magda-zielinska wants to merge 5 commits intoscikit-learn:masterfrom
magda-zielinska:permutation_importance_example
Closed

[MRG] DOC add an example for permutation-importance#16223
magda-zielinska wants to merge 5 commits intoscikit-learn:masterfrom
magda-zielinska:permutation_importance_example

Conversation

@magda-zielinska
Copy link
Copy Markdown
Contributor

Together with @fraboeni and @lopusz

Reference Issues/PRs

#3846

What does this implement/fix? Explain your changes.

Added an example to permutation importance

@wimlds
@adrinjalali
@noatamir

Copy link
Copy Markdown
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

A couple of changes

Number of Monte Carlo samples per original feature.
Equals the dimensionality of the computed feature space.

random_state : int, RandomState instance or None, optional (default=None)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you revert all the changes in the kernel_approximation file. It is not related with your PR.

LogisticRegression()

>>> result = permutation_importance(clf, X, y, n_repeats=10,
... random_state=42)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
... random_state=42)
... random_state=42)


>>> clf = LogisticRegression()
>>> clf.fit(X,y)
LogisticRegression()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
LogisticRegression()
LogisticRegression(...)

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

>>> clf = LogisticRegression()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
>>> clf = LogisticRegression()
>>> clf = LogisticRegression().fit(X, y)

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

>>> clf = LogisticRegression()
>>> clf.fit(X,y)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you can remove this line


>>> result = permutation_importance(clf, X, y, n_repeats=10,
... random_state=42)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could you print results before to check a key of the bunch

>>> clf = LogisticRegression()
>>> clf.fit(X,y)
LogisticRegression()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

remove the empty line as well

>>> clf.fit(X,y)
LogisticRegression()

>>> result = permutation_importance(clf, X, y, n_repeats=10,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
>>> result = permutation_importance(clf, X, y, n_repeats=10,
>>> result = permutation_importance(clf, X, y, n_repeats=2,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We want to have an execution time very small so 2 will be enough.

@glemaitre glemaitre changed the title [MRG] Permutation importance example #3846 [MRG] DOC add an example for permutation-importance Jan 26, 2020
@cmarmo
Copy link
Copy Markdown
Contributor

cmarmo commented Feb 5, 2020

Hi @magda-zielinska , could you find some time to address the reviewer comments? Thanks for participating to the sprint!

@magda-zielinska
Copy link
Copy Markdown
Contributor Author

Hello @cmarmo, I will take care of it in the next days! Thanks a lot!

@glemaitre
Copy link
Copy Markdown
Member

gentle ping @magda-zielinska

@magda-zielinska
Copy link
Copy Markdown
Contributor Author

good mornig everybody, apologies that this took so long!
now i see that changes to kernel_approximation.py were made by fraboeni so I cannot revert it.
I will close this PR and open another one.

@glemaitre
Copy link
Copy Markdown
Member

Do not hesitate to ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants