Skip to content
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

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

Conversation

tecosaur
Copy link

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.

@ablaom
Copy link
Member

ablaom commented Jun 26, 2022

@tecosaur Thanks indeed for this. I expect we will have some merge conflicts with the large PR #166 which is almost done. Let's rebase this PR after that and then review. Apologies in advance for the extra work.

@tecosaur
Copy link
Author

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).

@tecosaur
Copy link
Author

tecosaur commented Aug 2, 2022

@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.

@tecosaur
Copy link
Author

@ablaom I'm guessing you don't have much time to spend on this?

@ablaom
Copy link
Member

ablaom commented Oct 21, 2022

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.

@tecosaur
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants