Firstly I wanted to say that Treelite has been very useful, thanks! Would like to see a common decision tree format adopted more widely...
But - we have hit a small bug for LightGBM models with very deep leaves, say ones at a depth of ~35. (This is not a dense decision tree, of course! But if you grow leafwise, this can happen.) When you try to convert to Treelite, for example to use tl2cgen, you get an error:
booster: lgb.Booster
model = treelite.frontend.from_lightgbm(booster)
Debugging reveals that this is caused by overflow of the 32-bit signed ints in lightgbm.cc used to store node indexes. What goes wrong is that the "new node order" derived from "BFS order if it were a dense tree" gets too large too quickly.
One potential solution would be to use depth-first order to calculate a valid but much smaller Treelite node ordering. I have a PR that does that: #570 (Alternatively one could convert to "long long"/int64_t for indexes throughout the builder interfaces, but I feel like this would be far more disruptive for little benefit.) What do you think?
I've not contributed to this library before so please let me know what I can do to can help with a fix.
Thanks,
Tom
Firstly I wanted to say that Treelite has been very useful, thanks! Would like to see a common decision tree format adopted more widely...
But - we have hit a small bug for LightGBM models with very deep leaves, say ones at a depth of ~35. (This is not a dense decision tree, of course! But if you grow leafwise, this can happen.) When you try to convert to Treelite, for example to use tl2cgen, you get an error:
=>
Debugging reveals that this is caused by overflow of the 32-bit signed ints in lightgbm.cc used to store node indexes. What goes wrong is that the "new node order" derived from "BFS order if it were a dense tree" gets too large too quickly.
One potential solution would be to use depth-first order to calculate a valid but much smaller Treelite node ordering. I have a PR that does that: #570 (Alternatively one could convert to "long long"/int64_t for indexes throughout the builder interfaces, but I feel like this would be far more disruptive for little benefit.) What do you think?
I've not contributed to this library before so please let me know what I can do to can help with a fix. Thanks, Tom