WIP: Unify dump_model output (fixes #5962)

[thatlittleboy]

Closes #5962.

Note: PR is not ready for review yet. Still iterating on this, but I'm open to suggestions at this stage if any. Thank you!

Changes

• Added leaf_count to the string output of Tree::ToJSON() when num_leaves=1.

[thatlittleboy]

@microsoft-github-policy-service agree

[thatlittleboy]

Hi @jameslamb , I gave it a shot with the changes in this PR, but after re-installing and running pytest

sh build-python.sh install
pytest tests/python_package_test/test_engine.py::test_dump_model

I was still getting segmentation fault's, and I'm not quite sure how to debug. Swapping out leaf_count_[0] in the str_buf call for a literal number like 777 works, so the error is definitely arising because of leaf_count_[0].

For a stump tree, is leaf_count_ or internal_count_ undefined?

[jameslamb]

For a stump tree, is leaf_count_ or internal_count_ undefined?

I'm traveling this week and it may be a while until I'm able to look into this and help you, sorry. Maybe try tracing through the code with something like git grep leaf_count_ to find where it is set.

[thatlittleboy]

Update

An update: I've fixed the segmentation fault, but the leaf_count_ output is currently 0, whereas I was expecting it to be the number of samples that was used to train the booster.

Will try to look into this.

---

Notes for myself

Code tracing for the dump_model() function

bst.dump_model()
-> python-package/lightgbm/basic.py :: L3984  [definition of `dump_model`]
-> python-package/lightgbm/basic.py :: L4027  [call into `_safe_call(_LIB.LGBM_BoosterDumpModel`]
-> src/c_api.cpp :: L2491  [definition of `LGBM_BoosterDumpModel`, which does `ref_booster->DumpModel` internally]
-> src/boosting/gbdt_model_text.cpp :: L21  [definition of `DumpModel`]
-> src/boosting/gbdt_model_text.cpp :: L96  [which calls models_[i]->ToJSON()]

[jameslamb]

I just merged the latest changes from master into this. I'm ready to try to help... are you still interested in pursuing this, @thatlittleboy ?

[thatlittleboy]

@jameslamb Yep I'm interested still to follow up on this, but havent' had much progress since the last update.

TBH it might take a couple of weekends for me to get around to this -- I'll need to re-review where I got stuck and ask more specific questions here (haven't managed to find the few hours needed to sit down and concentrate on this ; been quite busy lately).

[jameslamb]

This was fixed by #6569, so closing this PR. @thatlittleboy I did tag you there... if you look at those changes (or the current state of LightGBM in general) and find that it does not fix the information for stumps in the way you'd wanted, please open a new GItHub issue and let us know.