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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

dragonbra
Copy link

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 <[email protected]>
@dragonbra
Copy link
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.

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

Successfully merging this pull request may close these issues.

4 participants