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

Code Refactoring #2341

Closed
1 of 6 tasks
guolinke opened this issue Aug 20, 2019 · 7 comments
Closed
1 of 6 tasks

Code Refactoring #2341

guolinke opened this issue Aug 20, 2019 · 7 comments

Comments

@guolinke
Copy link
Collaborator

guolinke commented Aug 20, 2019

Thanks to the active community, LightGBM receives many PRs for the new features.
Consequently, some codes become missing the readability and maintainability, for the coupling of many different functions.
For the convenience of future developments and friendliness for new users, I would like to have a code refactoring.

ping @chivee @StrikerRUS @jameslamb @Laurae2 @huanzhang12 @henry0312

you can add the files, which you think needs to refactor, to this issue.

  • Cost efficient gradient boosting
  • merge the logic of subset and subfeature
  • remove the macro functions, maybe convert them to template functions
  • refine the tree learning pipelines
  • refine prediction codes
  • remove the duplicate codes in **_bin
@StrikerRUS
Copy link
Collaborator

@guolinke Can we tick serial_tree_learner.cpp? Or do you want to do more refactoring?

@lcsdavid
Copy link
Contributor

May I add that there is an error in
<LightGBM/include/LightGBM/dataset_loader.h>:30:26-48
CostructFromSampleData should be ConstructFromSampleData ?

Regards.

@guolinke
Copy link
Collaborator Author

@lcsdavid I think it is a typo. welcome to create PR to fix it.

@StrikerRUS
Copy link
Collaborator

I'd like to propose refactoring additional config structures introduced in the initial CUDA support (I believe they can be replaced by ordinary config object)

enum LGBM_Device {
lgbm_device_cpu,
lgbm_device_gpu,
lgbm_device_cuda
};
enum Use_Learner {
use_cpu_learner,
use_gpu_learner,
use_cuda_learner
};
namespace LightGBM {
class LGBM_config_ {
public:
static int current_device; // Default: lgbm_device_cpu
static int current_learner; // Default: use_cpu_learner
};

#3160 (comment)

@jameslamb
Copy link
Collaborator

It's been a few years since the last comment on this issue. I think we should close it, what do you think @guolinke ?

@guolinke
Copy link
Collaborator Author

guolinke commented Feb 1, 2023

Sure, let us close it for now

@jameslamb
Copy link
Collaborator

thank you very much!

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

No branches or pull requests

4 participants