Refactoring monotone constraints (linked to #2305) #2717
Conversation
CharlesAuguste
requested review from
chivee,
guolinke and
huanzhang12
as code owners
CharlesAuguste
changed the title
| #ifndef LIGHTGBM_TREELEARNER_MONOTONE_CONSTRAINTS_H_ | ||
| #define LIGHTGBM_TREELEARNER_MONOTONE_CONSTRAINTS_H_ | ||
|
|
||
| #include <algorithm> |
There was a problem hiding this comment.
@CharlesAuguste include vector and cstdint?
|
As #2699 will significantly improve training efficiency. Could you wait for its merge? it is almost ready, except the GPU codes. |
CharlesAuguste
force-pushed
the
PR1-refactor-code-to-prepare-for-monotone-constraints
branch
from
63dbcf4 to
5ce7bdd
Compare
|
@guolinke I just rebased on your last PR. I also improved the compilation of this PR, and now only GPU is failing (but that's to be expected because I was not able to fix the file as explained in my initial comment). Let me know what you think. Thanks |
|
@CharlesAuguste I create PR in your repo |
|
@guolinke I merged your fix, we pass many more checks now. Thanks for that! However, there are still a few checks that we are not passing. These seem related to the docs and to the R package, but I don't understand how that is related to this PR. Any idea of what might be wrong? Thanks |
|
@CharlesAuguste I think they are temporary fails. |
|
Yea, it looks like some docs-related server is down, causing temporary connection errors. |
I just re-run the test. Now everything is OK. Sorry about the inconvenience. For R tests please refer to #2748 (comment). |
@StrikerRUS @CharlesAuguste @aldanor I think #2752 will fix the R test issues, let's see. |
|
@CharlesAuguste could you fix the conflicts? |
|
No problem, I will do fix them. |
CharlesAuguste
force-pushed
the
PR1-refactor-code-to-prepare-for-monotone-constraints
branch
from
57f427c to
a195864
Compare
|
@guolinke I solved the merge conflicts, and all checks seem to pass. So it should be ready to merge if you are happy enough with the PR now :). |
#2770) * Add util functions. * Added monotone_constraints_method as a parameter. * Add the intermediate constraining method. * Updated tests. * Minor fixes. * Typo. * Linting. * Ran the parameter generator for the doc. * Removed usage of the FeatureMonotone function. * more fixes * Fix. * Remove duplicated code. * Add debug checks. * Typo. * Bug fix. * Disable the use of intermediate monotone constraints and feature sampling at the same time. * Added an alias for monotone constraining method. * Use the right variable to get the number of threads. * Fix DEBUG checks. * Add back check to determine if histogram is splittable. * Added forgotten override keywords. * Perform monotone constraint update only when necessary. * Small refactor of FastLeafConstraints. * Post rebase commit. * Small refactor. * Typo. * Added comment and slightly improved logic of monotone constraints. * Forgot a const. * Vectors that are to be modified need to be pointers. * Rename FastLeafConstraints to IntermediateLeafConstraints to match documentation. * Remove overload of GoUpToFindLeavesToUpdate. * Stop memory leaking. * Fix cpplint issues. * Fix checks. * Fix more cpplint issues. * Refactor config monotone constraints method. * Typos. * Remove useless empty lines. * Add new line to separate includes. * Replace unsigned ind by size_t. * Reduce number of trials in tests to decrease CI time. * Specify monotone constraints better in tests. * Removed outer loop in test of monotone constraints. * Added categorical features to the monotone constraints tests. * Add blank line. * Regenerate parameters automatically. * Speed up ShouldKeepGoingLeftRight. Co-authored-by: Charles Auguste <[email protected]> Co-authored-by: guolinke <[email protected]>
…, microsoft#2717) (microsoft#2770) * Add util functions. * Added monotone_constraints_method as a parameter. * Add the intermediate constraining method. * Updated tests. * Minor fixes. * Typo. * Linting. * Ran the parameter generator for the doc. * Removed usage of the FeatureMonotone function. * more fixes * Fix. * Remove duplicated code. * Add debug checks. * Typo. * Bug fix. * Disable the use of intermediate monotone constraints and feature sampling at the same time. * Added an alias for monotone constraining method. * Use the right variable to get the number of threads. * Fix DEBUG checks. * Add back check to determine if histogram is splittable. * Added forgotten override keywords. * Perform monotone constraint update only when necessary. * Small refactor of FastLeafConstraints. * Post rebase commit. * Small refactor. * Typo. * Added comment and slightly improved logic of monotone constraints. * Forgot a const. * Vectors that are to be modified need to be pointers. * Rename FastLeafConstraints to IntermediateLeafConstraints to match documentation. * Remove overload of GoUpToFindLeavesToUpdate. * Stop memory leaking. * Fix cpplint issues. * Fix checks. * Fix more cpplint issues. * Refactor config monotone constraints method. * Typos. * Remove useless empty lines. * Add new line to separate includes. * Replace unsigned ind by size_t. * Reduce number of trials in tests to decrease CI time. * Specify monotone constraints better in tests. * Removed outer loop in test of monotone constraints. * Added categorical features to the monotone constraints tests. * Add blank line. * Regenerate parameters automatically. * Speed up ShouldKeepGoingLeftRight. Co-authored-by: Charles Auguste <[email protected]> Co-authored-by: guolinke <[email protected]>
@aldanor @redditur
@guolinke @chivee @jameslamb @huanzhang12 @Laurae2 @StrikerRUS
You were all reviewers of the PR #2305. This PR was judged to be not merge-able because it is too big. Therefore, in my ultimate comment (#2305 (comment)) I said I would split into smaller PR easier to merge. This is the first PR in relation with #2305.
The goal of this PR is just to refactor the current code around monotone constraints. The behavior of the library should not be affected by this PR. Refactoring the monotone constraints that way was asked by @guolinke in the previous PR (from the comment #2305 (comment)). We think that refactoring is very important because it will make it easier to develop more efficient constraining methods.
The main issue with this PR is that I was not able to refactor the files data_parallel_tree_learner.cpp, gpu_tree_learner.cpp, voting_parallel_tree_learner because I am not familiar with them (already mentioned in #2305 (comment) and in the discussion below). I can spend more time on it, but I would really appreciate getting help on that specific part if that is possible for you. @guolinke Since this refactor is something you were looking forward to, are you willing to fix these files for us? If you are familiar with these tree learners, I hope that would not be too much work. Please let me know what you think. Thank you very much.