-
Notifications
You must be signed in to change notification settings - Fork 102
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
Make prediction with probability free #180
base: dev
Are you sure you want to change the base?
Conversation
It looks like a bunch of issues come up with this design from the tests, I just haven't run into any of them in my usage. Regardless, now this PR is here you can see what I'm doing and potentially help with the change (I only have so much time I can spend on this). |
@ablaom I've found this codebase a little awkward to work with 😅 but I've managed to resolve the merge conflicts. It would be good if someone else could take a look at this and help move it forwards. |
@ablaom I'm guessing you don't have much time to spend on this? |
I don't doubt that you have made valuable progress here. However, as my time is limited, my modus operandi must be to only review PRs that pass tests, I'm afraid. |
I feared that that might be the case. Unfortunately I don't think I have the time to work this out by myself either. I guess we can hope for a helpful third party to come along at some point. The ~10x prediction performance improvement would be rather nice to have. |
This adds the change mentioned in #159, removing the (quite large) performance discrepancy between predicting with and without probability. I'd like to see the evaluation of decision paths be improved, but I haven't had the time to really dive into that so I may as well just upstream this improvement for now.
In testing I've seen a ~10x improvement in prediction time, and similar improvements in memory usage. See https://tecosaur.com/public/treeperf.html#observation-1 for more information.