-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add C backend for bayesian optimization #47
base: master
Are you sure you want to change the base?
Conversation
Note that this C backend is currently not really adapted for HyperMapper and is only plugged in through a very crude wrapper. The C backend is also currently very limited in its functionality. More functionality can be added later. Thus, work to be done includes tweaking the backend + wrapper to fit better into the rest of HyperMapper, and add functionality present in the other optimizer implementations.
Hi guys. |
I should also mention the specifics of what this C backend currently supports:
I think those are the most important restrictions that may need to be addressed before potentially merging. |
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.
Code looks good, to my somewhat limited understanding of C. Left just some minor comments. I'll comment on the features and overall thoughts in a separate comment.
Good job!
c_src/bayesian.c
Outdated
RandomForest forest; | ||
randomforest_fit(&forest, NUMBER_OF_TREES, points, dims, i); | ||
|
||
optimize_forest(&forest, lower, upper, points[i]->x, dims, best_val, workspace_points); |
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.
What is this optimize forest part? Is it the acquisition function optimization?
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.
Yes!
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.
It'd be better to have a more standard BO naming here. Can we simply call it acquisition function or similar?
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.
Renamed optimize_forest() -> optimize_acquisition_function()
Overall, I think it looks really good, and I really like this C backend initiative. Good job! I think the acquisition function optimization will be changed a bit based on a meeting we had earlier today, but other than that the rest seems fine. Some higher-level takes:
Commenting on the not-yet-implemented features:
There are some other (many of them minor) features of HM that are not implemented. To keep track (correct me if I'm wrong):
Most of these are minor features that I think we can leave for later, with really low priority, I'm adding it here mostly to have it written down for future reference. The exception may be constrained BO, which might be not very hard to add and is an often-needed feature. Prior injection and GPs are nice, but will probably be tricky to add and I think are not mandatory. I think the things I'd vote we work on next would be:
and later down the line:
@luinardi what do you think? |
Thank you for your comments @arturluis. Originally when discussing this C-backend with @luinardi, I believe the idea was for it to mostly be a smaller edge-case kind of thing. Essentially just a small, fast backend you can use if it fits your application. So I'm not sure how much effort we're willing to put into this? Of course we should add things like client-server and CSV-logging, but perhaps discuss the more advanced additions like GP model and prior injection etc. That being said, some of the extra changes you mentioned should be pefectly feasible without too much work. A GP model might be feasible for me to add. I'm currently reading up on GPs anyway, so if I have the time I might just implement it in C. All in all I definitely agree that we start with client-server, CSV-logging, and the acquisition-optimization. |
Good to know, I left those as my impressions and overall thoughts, even for future reference in case we want to expand this in the future. Even then, some of these features (like GPs and prior injection) we may want to leave for really long down the line. For now, though, I'm fully on board with having this as an edge case for specific scenarios. I'm not sure how much effort we should/want to to put into this, but for a minimal version, I think we just need the acquisition optimization and CSV results. To be honest, for a truly minimal backend, we can just have the CSV results and use the same acquisition function optimization as the Python implementation. The rest I'd say are all nice features, but not requirements. Cheers! |
Update to treat DoE as separate from optimization_iterations. Rename optimize_forest() -> optimize_acquisition_function(). Separate "use_c_backend" config parameter.
When changing to treat DoE and optimization_iterations as separate, I accidentally messed up indexing so DoE samples were overwritten. This change fixes that bug.
Note that this C backend is currently not really adapted for HyperMapper
and is only plugged in through a very crude wrapper.
The C backend is also currently very limited in its functionality.
More functionality can be added later.
Thus, work to be done includes tweaking the backend + wrapper to fit
better into the rest of HyperMapper, and add functionality present in
the other optimizer implementations.