Skip to content

[MRG] FEA Add support for Individual Conditional Expectation (ICE) plots #16619

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 149 commits into from
Jun 15, 2020
Merged

[MRG] FEA Add support for Individual Conditional Expectation (ICE) plots #16619

merged 149 commits into from
Jun 15, 2020

Conversation

madhuracj
Copy link
Contributor

@madhuracj madhuracj commented Mar 3, 2020 •
edited
Loading

This PR adds support for Individual Conditional Expectation (ICE) plots.

Reference Issues/PRs

Fixes #14126
Closes #16164

What does this implement/fix? Explain your changes.

After the discussion in #16164, it was decided to implement support for ICE plots with a parameter names individual. Since the decided method is much different from what's implemented in #16164, I am starting afresh.

Any other comments?

@NicolasHug
Copy link
Member

cool, please ping me when ready @madhuracj

madhuracj added 17 commits March 4, 2020 15:04
@madhuracj
Fix formatting error in method doc string
147cd13
@madhuracj
Unit tests for output shape of partial_dependence() method with 'indi…
3fedaa6 
…vidual' parameter
@madhuracj
Unit tests for the error raised when method='recursion' used with ind…
7d212ac 
…ividual=True/'both'
@madhuracj
FIx code formatting
0b2aaa2
@madhuracj
Two-way dependence plots are not allowed when individual=True/'both'
b8392cb
@madhuracj
Unit tests for axes_, lines_ and contours_ of PartialDependenceDispla…
0aa114e 
…y with different 'individual' values
@madhuracj
Specify the version parameter 'individual' was added
a85d036
@madhuracj
Update PD plot example with ICE plotting details
77f6fea
@madhuracj
Improve PDP/ICE example
3470f79
@madhuracj
Add an analysis of ICE plots
12f36db
@madhuracj
Fix the shape of lines_ and update unit tests to test it
6dd4ce9
@madhuracj
Documentation for Individual conditional expectation (ICE)
638d000
@madhuracj
Improve description of ICE behaviour
3a4b383
@madhuracj
Improve doc comments to include about ICE plots
33a105b
@madhuracj
Fix the example code by adding values for required 'features' parameter.
66085d4 
Use another estimator to make the example meaningful.
@madhuracj
Fix typo
647c518
@madhuracj
Fix line length
5b0872f
@madhuracj madhuracj changed the title [WIP] Add support for Individual Conditional Expectation (ICE) plots [MRG] Add support for Individual Conditional Expectation (ICE) plots Mar 7, 2020
@madhuracj
Copy link
Contributor Author

@NicolasHug @glemaitre @jnothman I think this PR is ready to be reviewed.
However, I am not sure what is causing an ImportError during unit tests on some platforms.

@glemaitre
Copy link
Member

You are probably missing a check_matplotlib somewhere. I will give a look soonish.

@madhuracj
Copy link
Contributor Author

Looks like the exception is thrown within check_matplotlib when it tries to import matplotlib.

@glemaitre glemaitre self-requested a review March 11, 2020 09:04
glemaitre
glemaitre reviewed Mar 11, 2020
View reviewed changes
Copy link
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 first pass with 2 questions regarding the API.
It looks good. I will have a thorough review this afternoon including the tests.

cmarmo
cmarmo reviewed Jun 10, 2020
View reviewed changes
cmarmo
cmarmo reviewed Jun 10, 2020
View reviewed changes
cmarmo
cmarmo reviewed Jun 10, 2020
View reviewed changes
cmarmo
cmarmo reviewed Jun 10, 2020
View reviewed changes
madhuracj and others added 6 commits June 11, 2020 08:05
@madhuracj @cmarmo
Fix indentation
d610045 
Co-authored-by: Chiara Marmo <cmarmo@users.noreply.github.com>
@madhuracj @cmarmo
Fix indentation
2cc9491 
Co-authored-by: Chiara Marmo <cmarmo@users.noreply.github.com>
@madhuracj @cmarmo
Fix indentation
5c924c9 
Co-authored-by: Chiara Marmo <cmarmo@users.noreply.github.com>
@madhuracj
Merge branch 'master' into ice2
f2546d3
@madhuracj
Fix documentation rendering - attempt 2
630c858
@madhuracj
This is the description
0c932ee
@madhuracj madhuracj requested review from glemaitre and cmarmo June 11, 2020 02:44
cmarmo
cmarmo approved these changes Jun 11, 2020
View reviewed changes
Copy link
Contributor

@cmarmo cmarmo left a comment

Choose a reason for hiding this comment

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

@madhuracj sphinx is finally happy I am too.. :)
@scikit-learn/core-devs, time to merge? Thanks!

@agramfort
Copy link
Member

example https://106969-843222-gh.circle-artifacts.com/0/doc/auto_examples/inspection/plot_partial_dependence.html looks good !

@glemaitre glemaitre merged commit 45a6ef7 into scikit-learn:master Jun 15, 2020
@glemaitre
Copy link
Member

OK let's go then. @madhuracj Thank you for the patience but it was worth.

@ogrisel in case you have some pending remarks that we did not catch during our reviews, feel free to open an issue and I will address it. It just needs to be 0.24 now :)

@glemaitre glemaitre removed their assignment Jun 15, 2020
@madhuracj
Copy link
Contributor Author

Thanks, @glemaitre. It was indeed a long journey. But I have learned a lot of specifics about contributing to scikit-learn. Hoping to keep contributing in the future.

@jnothman
Copy link
Member

jnothman commented Jun 17, 2020 via email

rubywerman pushed a commit to MLH-Fellowship/scikit-learn that referenced this pull request Jun 24, 2020
@madhuracj @rubywerman
FEA Add support for Individual Conditional Expectation (ICE) plots (s…
3c504f3 
…cikit-learn#16619)
viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020
@madhuracj @viclafargue
FEA Add support for Individual Conditional Expectation (ICE) plots (s…
4f9142e 
…cikit-learn#16619)
@glemaitre glemaitre mentioned this pull request Aug 18, 2020
Closed
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
@madhuracj @jayzed82
FEA Add support for Individual Conditional Expectation (ICE) plots (s…
5471547 
…cikit-learn#16619)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel left review comments

@NicolasHug NicolasHug NicolasHug left review comments

@jnothman jnothman jnothman approved these changes

@glemaitre glemaitre glemaitre approved these changes

+1 more reviewer

@cmarmo cmarmo cmarmo approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
module:inspection
Projects
None yet
Milestone
0.24
Development

Successfully merging this pull request may close these issues.

Add ICE to partial dependence plot
7 participants
@madhuracj @NicolasHug @glemaitre @jnothman @cmarmo @agramfort @ogrisel