-
Notifications
You must be signed in to change notification settings - Fork 11
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
Treelite JSON export: C++, R, Python implementation #144
Conversation
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.
Good progress so far, left some comments.
Python package name updated Rforestry -> random_forestry
pybind11 code simplified: forest* everywhere, removed excess functions Using py::return_value_policy::reference to prevent Python GC from deleting the trained tree object
Pushed Python part, no new tests were added, checked manually only. Haven't changed code much, because as I understand there is pending PR from @petrovicboban related to Python part soon. Few comments about current pybind11 C++, what can be simplified:
|
@Ilia-Shutov I'm very novice in C++, what you can see there is just my best effort. If there is anything that you think you can improve, feel free to submit PR, and if tests are still succeeding, you should go for it. Btw, we don't have unit tests in C++, that's something which should be done eventually. It seems also that you have domain knowledge, so feel free to explore sklearn compat branch, hopefully you can improve it. I'm devops guy only, working on CI/CD and general python, I don't have any DS domain skill and knowledge. |
Nice, if possible, let's add a basic Python test since the core function behavior should not change. Can you create a separate PR for the |
@edwardwliu Please don't merge yet, I've found a missing void* -> forestry* place which should be fixed or it will fail during serialisation/deserialisation, will push a commit soon UPD: fixed. Serialization/Deserialization test case is a part of the next PR related to sklearn |
@Ilia-Shutov saw the new commit, is this ready to squash and merge? |
exportTreeliteJson(C++), export_treelite_json(R) methods are introduced
New R package dependancy required for R tests running: rjson
linear forests are not supported - throwing an exception in that case.
Tested manually by importing and comparing prediction results using Python Treelite library (slight precision differences present)
R manual test script used:
Python manual test script used: