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.
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 #6569base: master
Are you sure you want to change the base?
[c++] Fix
dump_model()
information for root node #6569Changes from 16 commits
12102cc
c933399
c240016
2f1de57
273a1df
208df85
130879b
48e6b96
26b9859
88e3dec
e1274dc
38ee92c
3b423de
c89e257
3de14d9
fc42c1c
3ffcac6
d5a82c4
be7675d
f616e03
6c6bc33
c28a2cf
6113f90
94cf7f0
01aa952
fadaa83
b9c681b
a323acb
0fd0c59
4cc5dd4
a743a87
031c945
91993a9
f744f64
634b0fc
3fe4577
9e3e8ed
a01e737
e76d5bc
0af4631
04886c0
15fc3bf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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?