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

Jump to bottom

ENH Replace loss module HGBT #20811

Merged
merged 155 commits into from Jan 11, 2022
Merged

ENH Replace loss module HGBT #20811

merged 155 commits into from Jan 11, 2022
+328 ∄1�71,128

Conversation

@lorentzenchr
Copy link
Member

@lorentzenchr lorentzenchr commented Aug 22, 2021 •
edited

Reference Issues/PRs

Follow-up of #20567.

What does this implement/fix? Explain your changes.

This PR replaces the losses of HGBT in sklearn/ensemble/_hist_gradient_boosting with the new common loss module of #20567.

Any other comments?

Similar to #19089, but only HGBT.
This PR is based on 7bee26b.
Edit: #20567 was merged 🚀
lorentzenchr added 30 commits Jul 18, 2021
@lorentzenchr
ENH add common link function submodule
f8362d7
@lorentzenchr
ENH add common loss function submodule
afdb67e
@lorentzenchr
CLN replace deprecated np.int by int
830b814
@lorentzenchr
DOC document default=1 for n_threads
9504c89
@lorentzenchr
CLN comments and line wrapping
fb3bce2
@lorentzenchr
CLN comments and doc
2c86bf4
@lorentzenchr
BUG remove useless line of code
d68c07e
@lorentzenchr
CLN remove line that was commented out
3d9c800
@lorentzenchr
CLN nitpicks in comments and docstrings
aba1b67
@lorentzenchr
ENH set NPY_NO_DEPRECATED_API
022e418
@lorentzenchr
MNT change NPY_1_13_API_VERSION to NPY_1_7_API_VERSION
49bb402
@lorentzenchr
MNT comment out NPY_NO_DEPRECATED_API
6d77090
@lorentzenchr
TST restructure domain test cases
ceda673
@lorentzenchr
DOC add losses to API reference
c73e3fa
@lorentzenchr
MNT add classes to __init__
e650522
@lorentzenchr
CLN fix import
a31d8fb
@lorentzenchr
DOC minor docstring changes
e5b6266
@lorentzenchr
TST prefer docstring over comment
3492383
@lorentzenchr
ENH define loss.is_multiclass
9d86d82
@lorentzenchr
DOC fix typos
cc90e4d
@lorentzenchr
CLN address review comments
d0b48ac
@lorentzenchr
DOC small docstring improvements
7794617
@lorentzenchr
TST test more losses in test_specific_fit_intercept_only
35b7423
@lorentzenchr
FIX test_loss_boundary
b390002
@lorentzenchr
ENH Tempita for losses
12b4634
@lorentzenchr
MNT apply black
061a41b
@lorentzenchr
TST replace np.quantile by np.percentile
98f8877
@lorentzenchr
ENH make Interval a dataclass
3f8ffe9 
- function is_in_interval_range -> method Interval.includes
@lorentzenchr
DOC improve docstrings in link.py
b5e61d2
@lorentzenchr
MNT use numpy dtype instead of Python type
cfdd67c
@lorentzenchr
Copy link
Member Author

@lorentzenchr lorentzenchr commented Dec 5, 2021 •
edited

I can't reproduce the circleci failure locally. /home/circleci/project/doc/whats_new/v1.1.rst:338: WARNING: Title underline too short.'

@lorentzenchr
Copy link
Member Author

@lorentzenchr lorentzenchr commented Dec 11, 2021

Marking as high priority because it enables #21800 very easily.

thomasjpfan
thomasjpfan reviewed Dec 18, 2021
View changes
sklearn/_loss/tests/test_loss.py Outdated Show resolved Hide resolved
sklearn/_loss/loss.py Show resolved Hide resolved
sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py Outdated
if self._loss.constant_hessian:
self._loss.gradient(
y_true=y_train,
raw_prediction=raw_predictions.T,
Copy link
Member

@thomasjpfan thomasjpfan Dec 18, 2021 •
edited

Choose a reason for hiding this comment

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

Interesting, I thought there would have been some cpu cache performance issue with either "C" or "F" order. (Depending on which axis prange is iterating over)

sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py Outdated Show resolved Hide resolved
thomasjpfan
thomasjpfan reviewed Dec 18, 2021
View changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Overall looks good. There is some overhead with remembering that raw_predictions has shape: (n_trees_per_iteration, n_samples) and gradient and hessians has shape: (n_samples, n_trees_per_iteration).

Was there a reason that BaseLoss to prefer (n_samples, n_trees_per_iteration) over the reverse?

@lorentzenchr
Copy link
Member Author

@lorentzenchr lorentzenchr commented Dec 18, 2021 •
edited

Overall looks good. There is some overhead with remembering that raw_predictions has shape: (n_trees_per_iteration, n_samples) and gradient and hessians has shape: (n_samples, n_trees_per_iteration).

Was there a reason that BaseLoss to prefer (n_samples, n_trees_per_iteration) over the reverse?

TBH, that's just my preferred way of looking at it. This way, raw_predictions, X, y, predict() and predict_proba all have samples on the first axis (axis=0).

I could try to make it consistent and change all HGBT functions to use raw_prediction.shape=(n_samples, n_trees_per_iteration).

Edit: 27e818f is an attempt to do so. @thomasjpfan what do you think?

@thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Dec 18, 2021

Edit: 27e818f is an attempt to do so. @thomasjpfan what do you think?

Yea, I think everything is much nicer now. (The diff is surprising smaller than I thought it would be.)

thomasjpfan
thomasjpfan approved these changes Dec 19, 2021
View changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

LGTM

sklearn/_loss/loss.py Show resolved Hide resolved
sklearn/ensemble/_hist_gradient_boosting/tests/test_gradient_boosting.py Outdated Show resolved Hide resolved
jjerphan
jjerphan reviewed Jan 10, 2022
View changes
Copy link
Member

@jjerphan jjerphan left a comment

Thank you for this PR, @lorentzenchr.

Here are a few comments.

sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py Outdated Show resolved Hide resolved
sklearn/_loss/loss.py Outdated Show resolved Hide resolved
sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py Show resolved Hide resolved
sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py Outdated Show resolved Hide resolved
sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py Outdated Show resolved Hide resolved
sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py Outdated Show resolved Hide resolved
jjerphan
jjerphan approved these changes Jan 10, 2022
View changes
Copy link
Member

@jjerphan jjerphan left a comment •
edited

LGTM.

Edit: #22173 has been opened for a follow-up.

@lorentzenchr
Copy link
Member Author

@lorentzenchr lorentzenchr commented Jan 11, 2022

2 approvals 🎉
It seems custom that a reviewer merges. Is there a passage in the developer guideline that I missed or is it just custom to do so?

@thomasjpfan thomasjpfan changed the title [MRG] ENH Replace loss module HGBT ENH Replace loss module HGBT Jan 11, 2022
thomasjpfan
thomasjpfan reviewed Jan 11, 2022
View changes
doc/whats_new/v1.1.rst Outdated Show resolved Hide resolved
thomasjpfan
thomasjpfan approved these changes Jan 11, 2022
View changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

I did one more pass. This looks good for merging. Thank you for working on this!

@thomasjpfan thomasjpfan merged commit 4e974e0 into scikit-learn:main Jan 11, 2022
31 checks passed
@thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Jan 11, 2022

Is there a passage in the developer guideline that I missed or is it just custom to do so?

I think it's mostly a custom. I do not see it documented anywhere.

@lorentzenchr lorentzenchr deleted the loss_module_hgbt branch Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel

@NicolasHug NicolasHug

@thomasjpfan thomasjpfan

@jjerphan jjerphan

Assignees
No one assigned
Labels
cython High Priority module:ensemble Waiting for Reviewer
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet
5 participants
@lorentzenchr @thomasjpfan @ogrisel @NicolasHug @jjerphan