Skip to content
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

[MRG] Fast PDPs for histogram-based GBDT #13769

Merged
merged 28 commits into from Jul 11, 2019

Conversation

@NicolasHug
Copy link
Contributor

NicolasHug commented May 2, 2019

This PR implements fast partial dependence computation for the new histogram-based GBDTs.

Both BaseGradientBoosting and BaseHistGradientBoosting now have a _compute_partial_dependence_recursion() method.

The cython code for computing PDPs of the histogram-based predictors is very similar to that of the regular trees.

NicolasHug added 3 commits Apr 27, 2019
WIP
@glemaitre

This comment has been minimized.

Copy link
Contributor

glemaitre commented May 2, 2019

Ping me when this is ready to be reviewed ;-)

@NicolasHug NicolasHug changed the title [WIP] Fast PDPs for histogram-based GBDT [MRG] Fast PDPs for histogram-based GBDT May 3, 2019
@NicolasHug

This comment has been minimized.

Copy link
Contributor Author

NicolasHug commented May 3, 2019

Ready @glemaitre ;)

@glemaitre glemaitre self-requested a review May 6, 2019
@ogrisel

This comment has been minimized.

Copy link
Member

ogrisel commented May 9, 2019

I have pushed an updated example to use the HistGradientBoosting regressor in the PDP example.

I have also fixed the example to:

  • remove the main() function that is no longer needed with loky;
  • put the analysis inside the code;
  • make the MLPRegressor reach close to .82 R2 score to be comparable with the GBRT model (this requires Quantile based feature scaling + a deeper model + early stopping to avoid long fit times).

Here are the results:

Figure_1

Figure_2

You can observe that the Neural Net PDP now agrees with the GBRT PDB for all the features. It was not the case with a weaker model.

@NicolasHug

This comment has been minimized.

Copy link
Contributor Author

NicolasHug commented May 9, 2019

thanks!!

@ogrisel

This comment has been minimized.

Copy link
Member

ogrisel commented May 9, 2019

The new plots match Figure 10.16 (p 374) of ESL II even better than the PDP plots of the non-histogram based GBRT.

@ogrisel

This comment has been minimized.

Copy link
Member

ogrisel commented May 9, 2019

The test_fastica_simple failure is unrelated. I will open a PR to make it deterministic.

Copy link
Member

ogrisel left a comment

Here a bunch of minor comments:

sklearn/inspection/partial_dependence.py Outdated Show resolved Hide resolved
sklearn/ensemble/gradient_boosting.py Outdated Show resolved Hide resolved
@ogrisel

This comment has been minimized.

Copy link
Member

ogrisel commented May 9, 2019

In the plots of the example above, I find it weird that the MLPRegressor partial dependence values are all shifted by +2 w.r.t. the GBRT values. If I switch to method="brute" I also get the shift by +2.

@ogrisel

This comment has been minimized.

Copy link
Member

ogrisel commented May 9, 2019

I found the cause of the shift: the offset in y is supposed to happen before the train / test split, otherwise it's either not taken into account or the r2 score cannot be interpreted easily :)

ogrisel added 3 commits May 9, 2019
This reverts commit f0f8641.

Actually viridis is already good enough on recent matplotlib versions
and we want to continue supporting older matplotlib versions.
@ogrisel

This comment has been minimized.

Copy link
Member

ogrisel commented May 10, 2019

Here is the rendering of the example:

https://58079-843222-gh.circle-artifacts.com/0/doc/auto_examples/inspection/plot_partial_dependence.html#sphx-glr-auto-examples-inspection-plot-partial-dependence-py

One can see that the links to the sklearn.ensemble.HistGradientBoostingRegressor class do not work because of the experimental setup. This is unexpected because the line:

from sklearn.experimental import enable_hist_gradient_boosting  # noqa

is present in doc/conf.py.

The same problem appears in the API table of contents:

https://58079-843222-gh.circle-artifacts.com/0/doc/modules/classes.html#module-sklearn.ensemble

@ogrisel

This comment has been minimized.

Copy link
Member

ogrisel commented May 10, 2019

This is probably caused by #13824 that was merged to master concurrently. Let me try to merge master and fix in this PR.

Copy link
Member

ogrisel left a comment

I addressed my own nitpicks. LGTM.

NicolasHug added 3 commits May 25, 2019
…ikit-learn into fast_partial_dep_hist_gbdt
…st_partial_dep_hist_gbdt
@ogrisel

This comment has been minimized.

Copy link
Member

ogrisel commented Jul 3, 2019

I believe this PR is ready to merge. @glemaitre any further comment?

@glemaitre

This comment has been minimized.

Copy link
Contributor

glemaitre commented Jul 3, 2019

@ogrisel

This comment has been minimized.

Copy link
Member

ogrisel commented Jul 3, 2019

They already are two images. It's sphinx-gallery that's displaying them side by side. But I agree this could be improved by having two code blocks: one for MLPRegressor and one for GBRT. Each figure would appear under the matching code block and that should improve readability.

@NicolasHug

This comment has been minimized.

Copy link
Contributor Author

NicolasHug commented Jul 5, 2019

@amueller

This comment has been minimized.

Copy link
Member

amueller commented Jul 5, 2019

Can you use tight layout or constraint layout to make the ylabels not overlap the plots?

@NicolasHug

This comment has been minimized.

Copy link
Contributor Author

NicolasHug commented Jul 5, 2019

The new one looks slightly better, though it's hard to find something that works both for local and sphinx plots.

@glemaitre glemaitre merged commit 0eaaeaf into scikit-learn:master Jul 11, 2019
17 checks passed
17 checks passed
LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python 3 new alerts
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 96.82%)
Details
codecov/project Absolute coverage decreased by -0.61% but relative coverage increased by +3.17% compared to ec6d0bb
Details
scikit-learn.scikit-learn Build #20190705.31 succeeded
Details
scikit-learn.scikit-learn (Linux py35_conda_openblas) Linux py35_conda_openblas succeeded
Details
scikit-learn.scikit-learn (Linux py35_np_atlas) Linux py35_np_atlas succeeded
Details
scikit-learn.scikit-learn (Linux pylatest_conda) Linux pylatest_conda succeeded
Details
scikit-learn.scikit-learn (Linux32 py35_np_atlas_32bit) Linux32 py35_np_atlas_32bit succeeded
Details
scikit-learn.scikit-learn (Windows py35_32) Windows py35_32 succeeded
Details
scikit-learn.scikit-learn (Windows py37_64) Windows py37_64 succeeded
Details
scikit-learn.scikit-learn (macOS pylatest_conda) macOS pylatest_conda succeeded
Details
@glemaitre

This comment has been minimized.

Copy link
Contributor

glemaitre commented Jul 11, 2019

The rendering is ok for now. Thanks @NicolasHug

koenvandevelde added a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.