-
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
[c++] Fix dump_model()
information for root node
#6569
Open
neNasko1
wants to merge
42
commits into
microsoft:master
Choose a base branch
from
neNasko1:fix-root-values
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 8 commits
Commits
Show all changes
42 commits
Select commit
Hold shift + click to select a range
12102cc
Fix value calculation in root node
c933399
Fix dask tests
c240016
Merge branch 'master' into fix-root-values
neNasko1 2f1de57
Create proper tests
273a1df
Merge branch 'master' into fix-root-values
neNasko1 208df85
Test only on cpu
130879b
Merge branch 'fix-root-values' of github.com:neNasko1/LightGBM into f…
48e6b96
Disable new tests for CUDA
26b9859
Merge with #5964
88e3dec
Finish merging with dump_model unification
e1274dc
Improve tests
38ee92c
Add linear test for stump
3b423de
Fix CUDA compilation
c89e257
Merge branch 'master' into fix-root-values
neNasko1 3de14d9
Merge branch 'master' into fix-root-values
neNasko1 fc42c1c
Merge branch 'master' into fix-root-values
neNasko1 3ffcac6
Comments after code review
d5a82c4
Fix test
be7675d
Reenable cuda testing
f616e03
Tests
6c6bc33
Merge branch 'microsoft:master' into fix-root-values
neNasko1 c28a2cf
test cuda
6113f90
.
94cf7f0
Fix warning
01aa952
reenable tests
fadaa83
.
b9c681b
Merge branch 'fix-cuda' into fix-root-values
a323acb
fix cuda
0fd0c59
Fix compilation error
4cc5dd4
Fix weight
a743a87
Fix numerical
031c945
Make tests more robust
91993a9
Merge branch 'master' into fix-root-values
neNasko1 f744f64
Merge branch 'master' into fix-root-values
neNasko1 634b0fc
Fix test failing because of accuracy reasons
3fe4577
Fix test_dask::test_init_scores
9e3e8ed
Decrease size of trees in test
a01e737
Merge branch 'master' of github.com:microsoft/LightGBM into fix-root-…
jameslamb e76d5bc
add a test on predictions from a model of all stumps
jameslamb 0af4631
Comments after code review
neNasko1 04886c0
Small text QOL
neNasko1 15fc3bf
Add test_predict_stump on dask
neNasko1 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
Oops, something went wrong.
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.
Can you help me understand why this test had to be changed, when in
gbdt.cpp
I see the following?And if this is no longer the case... can you please help us come up with a better way to change this test? This particular test is called
test_init_score
.... its specific goal is to check that when you provideinit_score
to the.fit()
method for the Dask estimators, thatinit_score
actually makes it all the way down to the Booster and affects the resulting model.Changing this condition to just "model fitting did not raise an error" is not sufficiently strict... that test would pass if
DaskLGBMRegressor.fit()
simply ignoredinit_score
, for example.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.
Reverted!
Nice catch, seems like it got fixed in the meantime.
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 may have misstested. I made the test more comprehensive, however it now does not execute on
LGBMRanker
. However this is out-of-scope for this PR.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.
If the changes in your PR require tests that were previously passing to now be skipped, the need to skip those tests is not "out-of-scope".
Can you please explain here what issues you saw with
LGBMRanker
and this PR's changes?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.
The way the
init_score
was tested seems like a flawed way to test it, because as we see from this PR the root value was always 0.Rewriting the test to be "check if the 2 models are different" seems to have more merit, since if you have an init score you would expect that to happen with a small number of trees.
It seems however that is not the case in the
LGBMRanker
, i.e. when boosting from aninit_score
or without one the models are exactly the same. Could you tell me whether I am missing something and this is the expected functionality?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.
Based on all of this... I feel pretty good about these changes! I think this can be merged once the other suggestions I've left have been addressed.
One other note relevant to this thread... I just pushed a new unit test to this PR: e76d5bc
That tests that the predictions from a model of all stumps takes on the correct value based on whether or not
init_score
is provided.Right now if you train for 5 iterations but LightGBM cannot find any splits, you won't get 5 trees... you'll get 1 stump.
That unit test will protect us against predictions being wrong if that ever changes and if, for example, that situation produces 5 stumps. I'm thinking about this in-progress PR, for example: #5699.
cc @jmoralez @StrikerRUS (just for awareness)
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.
@neNasko1 I've copied your comment from #6569 (comment) here ... let's please keep this in a thread, so there's only one place to link to and so it takes up less visual space scrolling through the already-very-long activity history for this PR.
I said:
And you asked:
Yes,
boost_from_average
is a part of this. That parameter is set totrue
by defaulthttps://github.com/microsoft/LightGBM/blob/06432300c0c01268c8a80c3537eef81dd5ede30d/include/LightGBM/config.h#L946-947
And yes it's correct that when you provide an
init_score
, then boosting from the average doesn't occur, and the first tree's root node has an internal value of0.0
.Which test do you mean "this test" and what specific changes do you think are needed?
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 addressing the changes to the
test_dask::test_init_scores
. As we saw previously the test was properly checking if ainit_score
was supplied and propagated. However after this fix, this is no longer the case so a different way to test thetest_dask::test_init_scores
-s is needed.I have addressed the changes in one commit, since they were a lot and I wanted to make sure that they work properly together.
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.
Ohhhh I see. Yes I think you're right, that's a great point. Here's my proposal for making that test stricter:
params
intest_dask.py::test_init_scores
to eliminate other sources of non-determinism, so we can be more sure the difference is only due to init_score:test_predict_stump()
test I'd pushed: https://github.com/neNasko1/LightGBM/blob/04886c0d6d2da2e3d145916556ef34d012b43ec9/tests/python_package_test/test_engine.py#L3815-L3840What do you think about that? And please let me know if you're not very familiar with Dask and want me to push a test like that.
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 not that familiar with dask, but is this what you have in mind?
P.S. I am running the dask tests on my M3 Mac machine by removing these lines. Maybe it will be possible to enable
test_dask
for apple silicone in a future PR?