-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Pr3 monotone constraints splits penalization #2939
Merged
StrikerRUS
merged 26 commits into
microsoft:master
from
CharlesAuguste:PR3-monotone-constraints-splits-penalization
Apr 9, 2020
Merged
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
830c9bf
Add the monotone penalty parameter to the config.
6b0f7bd
Pass tree in the necessary functions so it can be used in ComputeBest…
1b98464
Add monotone penalty.
e662261
Added link to the original report.
4dc2c7a
Add tests.
ea3aab9
Fix GPU.
c79c10c
Revert "Pass tree in the necessary functions so it can be used in Com…
6e808cb
Revert "Fix GPU."
75a0b9a
Added a shared pointer to the tree so the constraints can use it too.
b976970
Moved check on monotone penalty to config.cpp.
1c1639f
Python linting.
a95188f
Use AssertTrue instead of assert_.
0e3fd27
Fix penalization in test.
aeda36b
Make GPU deterministic in tests.
ae0106f
Rename tree to tree_ in monotone constraints.
8bc4cfd
Replaced epsilon by kEplison.
7a30d2f
Typo.
0bd18c2
Make tree pointer const.
afbcda6
Update src/treelearner/monotone_constraints.hpp
bd7b037
Update src/treelearner/monotone_constraints.hpp
9e28547
Added alias for the penalty.
3a05420
Remove useless comment.
36acccb
Save CI time.
3ab294c
Refactor test_monotone_penalty_max.
fada0c6
Update include/LightGBM/config.h
782abe8
Fix doc to be in line with previous config change commit.
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a special function for floats comparison. Or is it a case when we need exact equality like here?
LightGBM/tests/python_package_test/test_basic.py
Lines 60 to 61 in ce08242
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am actually not sure here. I compare a model with 1 variable (the unconstrained one) to a model with 3 variables, with 2 of the 3 that should not be used at all because of penalization (this is the constrained model here). So in the end, the 2 resulting models should output exactly the same results I think, because a single variable is used in both to make splits. So I would say that assert_array_equal is what we want, but I am not entirely sure. Given my explanation which do you think is best?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with that one-feature model should output the same predictions. But I have a doubt that we won't have problems due to internal grad/hess float summation. However, I'm OK with the current code while CI is producing green status. At least, we are now aware about the place where to look if problems appear.