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

Decouple Boosting Types (fixes #3128) #4827

Merged
merged 105 commits into from
Dec 28, 2022
Merged

Conversation

lyf-00
Copy link
Contributor

@lyf-00 lyf-00 commented Nov 26, 2021

Fixes #3128.

Now GOSS and DART are mutual exclusive in LightGBM. Because they are both subclass of GBDT, only one of them can be instantiated during running.

However, GOSS is a data sampling strategy, DART is a tree weighting strategy, theoretically they can be set at the same time.
In this branch, we try to decouple GOSS and DART by abstracting GOSS as a DataSampleStrategy, implemented as a new class.

Also, Bagging in GBDT is a data sampling strategy. So we implement both Bagging and GOSS as subclasses of DataSampleStrategy.

architecture

@ghost
Copy link

ghost commented Nov 26, 2021

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ lyf-00 sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

@jameslamb jameslamb self-requested a review November 29, 2021 02:27
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks for your interest in LightGBM, and for this proposal!

The description of the desired behavior makes sense to me at a high level, but I think this deserves more discussion before proceeding with the work involved in updating and reviewing this pull request (note that right now these changes cause the Python tests to segfault).

I'm going to mark this "in progress", and wait to review the code changes until other maintainers (especially @shiyu1994) have a chance to comment on whether they support the overall idea of this pull request.

src/boosting/goss.hpp Outdated Show resolved Hide resolved
src/boosting/gbdt.cpp Outdated Show resolved Hide resolved
src/boosting/bagging.hpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@shiyu1994 shiyu1994 left a comment

Choose a reason for hiding this comment

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

Leave a few comments for now. I'll spend more time looking into this later today. Thanks for your contribution.

include/LightGBM/config.h Show resolved Hide resolved
include/LightGBM/sample_strategy.h Outdated Show resolved Hide resolved
include/LightGBM/sample_strategy.h Outdated Show resolved Hide resolved
include/LightGBM/sample_strategy.h Outdated Show resolved Hide resolved
src/boosting/gbdt.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Thanks a lot for getting back to this PR!

I don't have any additional comments. LGTM for Python tests.

@guolinke
Copy link
Collaborator

guolinke commented Oct 9, 2022

@shiyu1994 is this PR ready to merge?

@lyf-00
Copy link
Contributor Author

lyf-00 commented Dec 13, 2022 via email

@lyf-00
Copy link
Contributor Author

lyf-00 commented Dec 13, 2022 via email

@shiyu1994
Copy link
Collaborator

@lyf-00 the command you issued was incorrect. Please try again.

Examples are:

@microsoft-github-policy-service agree

and

@microsoft-github-policy-service agree company="your company"

@lyf-00 Could you please sign the license according to this example, thanks!

@shiyu1994
Copy link
Collaborator

@lyf-00 Extra characters are sent through email reply. So could you please issue the command through Github website when you have time? Thanks.

@lyf-00
Copy link
Contributor Author

lyf-00 commented Dec 28, 2022

@microsoft-github-policy-service agree

@shiyu1994 shiyu1994 merged commit fffd066 into microsoft:master Dec 28, 2022
@jameslamb
Copy link
Collaborator

wow!!! Thanks so much for coming back and finishing this @shiyu1994 !

I wish @guolinke had had a chance to review one more time, since the only approval on this was @StrikerRUS's from about 4 months ago and there were several commits pushed onto it after that.

Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decouple boosting types
7 participants