[c++] Fix dump_model() information for root node

[neNasko1]

This PR corrects the output of dump_model() and other dump-related functions like trees_to_dataframe(). There are 2 fixes implemented:

1. The current Tree::Split implementation incorrectly saves the old leaf output value in the internal_value_ array when called on the root node. This in turn makes inspecting the whole training process from python incomplete.

Before:

(Pdb) booster_.trees_to_dataframe()
     tree_index  node_depth node_index left_child right_child parent_index  ... decision_type  missing_direction missing_type     value weight count
0             0           1       0-S0       0-S1        0-S2         None  ...            ==              right         None  0.000000      0   200
1             0           2       0-S1       0-S5        0-S4         0-S0  ...            <=               left         None  0.106573    113   113
2             0           3       0-S5       0-L0        0-L6         0-S1  ...            ==              right         None  0.082122     56    56
3             0           4       0-L0       None        None         0-S5  ...          None               None         None  0.064612     26    26
4             0           4       0-L6       None        None         0-S5  ...          None               None         None  0.097297     30    30

After:

(Pdb) booster_.trees_to_dataframe().head()
   tree_index  node_depth node_index left_child right_child parent_index  ... decision_type  missing_direction missing_type     value weight count
0           0           1       0-S0       0-S1        0-S2         None  ...            ==              right         None  0.081757    200   200
1           0           2       0-S1       0-S5        0-S4         0-S0  ...            <=               left         None  0.106573    113   113
2           0           3       0-S5       0-L0        0-L6         0-S1  ...            ==              right         None  0.082122     56    56
3           0           4       0-L0       None        None         0-S5  ...          None               None         None  0.064612     26    26
4           0           4       0-L6       None        None         0-S5  ...          None               None         None  0.097297     30    30

2. Stump has no leaf_count inside dump_model() output #5962

[neNasko1]

Currently the CI is not passing as #6574 is blocking.

[neNasko1]

I am open to ideas of ways to test related functionalities.

Tests should now be sufficient for the change.

[neNasko1]

@jameslamb
Could you take a look at the PR, now that the CI is passing?

[jameslamb]

@shiyu1994 or @guolinke could you help with a review of this?

I'm not sure if this will correctly handle these cases:

• custom init_score provided (via Dataset)
• boost_from_average=False passed

@neNasko1 could you also look at #5962 and let us know if you think this change would fix the issue @thatlittleboy reported there?

[neNasko1]

Thank you for taking the time to look into the PR and linking a relevant issue.

I'm not sure if this will correctly handle these cases:

• custom init_score provided (via Dataset)
• boost_from_average=False passed

I think those cases are handled as the results are consistent with what leaf values report, I also remade the test to boost from average.

@neNasko1 could you also look at #5962 and let us know if you think this change would fix the issue @thatlittleboy reported there?

I took the liberty to merge @thatlittleboy's WIP code into mine, additionally fixing the issues that they reported. I will also change the description of the PR to reflect both of the fixes.

[guolinke]

Sorry for the late response. This PR looks good to me.

[jameslamb]

Thanks very much for the review @guolinke !

I've left a few other small requests.

[jameslamb]

Thanks, I left a few questions for your consideration.

[borchero]

This looks good to me! 🚀

[neNasko1]

@jameslamb
Just to recap: the tests/python_package_test/test_dask.py seems to have previously been a no-op since the 2 models produced with and without an init scores are the same for the classifier case. This however is not related to the current changes. Can you tell me whether I am missing something?

[jameslamb]

the tests/python_package_test/test_dask.py seems to have previously been a no-op since the 2 models produced with and without an init scores are the same for the classifier case.

I'll investigate this when I can, hopefully in the next few days. In the interim, you can help move this forward by resolving merge conflicts and pulling in the latest changes on master.

[StrikerRUS]

LGTM!

But I'll keep following the discussion about Dask Ranker test (#6569 (comment)).