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

[GPU] Add support for linear tree with device=gpu #6567

Merged
merged 21 commits into from
Oct 18, 2024

Conversation

dragonbra
Copy link
Contributor

GPU version's implementation refer to issue #6555

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.

@dragonbra Thanks a lot for your contribution!

I checked that there are no any other differences except "gpu_" prefixes in new files compared to original linear_tree_learner.h and linear_tree_learner.cpp.

I don't think that my review should be count to allow the merge. But I left one wording suggestion and would like to ask you add new files in LightGBM.vcxproj and LightGBM.vcxproj.filters: https://github.com/microsoft/LightGBM/tree/master/windows.

src/io/config.cpp Outdated Show resolved Hide resolved
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
@dragonbra
Copy link
Contributor Author

@dragonbra Thanks a lot for your contribution!

I checked that there are no any other differences except "gpu_" prefixes in new files compared to original linear_tree_learner.h and linear_tree_learner.cpp.

I don't think that my review should be count to allow the merge. But I left one wording suggestion and would like to ask you add new files in LightGBM.vcxproj and LightGBM.vcxproj.filters: https://github.com/microsoft/LightGBM/tree/master/windows.

Hi, I checked the files LightGBM.vcxproj and LightGBM.vcxproj.filters but I didn't see gpu_tree_learner.h in them. I wonder if I should add things like gpu_linear_tree_learner.h in them without gpu_tree_learner.h.

@StrikerRUS
Copy link
Collaborator

@dragonbra Thanks for checking this! Indeed, there are no any gpu_* files. So I think we shouldn't add your new files there as well.


namespace LightGBM {

void GPULinearTreeLearner::Init(const Dataset* train_data, bool is_constant_hessian) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Overall, the solution looks good to me. I've read half of the implementation and need some more time to check the correctness. Thank you.

For test case, I think the test case for this scenario is automatically enabled with existing test cases for linear trees and device_type=gpu.

@shiyu1994 shiyu1994 self-requested a review October 11, 2024 12:01
@shiyu1994
Copy link
Collaborator

Given the redundancy in gpu_linear_tree_learner.cpp and linear_tree_learner.cpp, I will try to merge these two files. Will push some changes to remove the duplicate code soon.

@shiyu1994 shiyu1994 merged commit c7d3ac1 into microsoft:master Oct 18, 2024
48 checks passed
@jameslamb
Copy link
Collaborator

Thank you very much for simplifying this to reduce the code duplication @shiyu1994 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants