-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 logging with state to Bayesian Optimizer #547
base: master
Are you sure you want to change the base?
Add logging with state to Bayesian Optimizer #547
Conversation
… acquisition function state
@adrianmolzon I realized the coverage bot is broken, so let me fix that first before moving on with this PR |
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.
Generally looks good, especially the extensive testing. I added a few comments here and there.
I think the old save/load functionality should be removed, it's better not to have multiple ways to do the same thing, especially if one of them is clearly superior.
bayes_opt/acquisition.py
Outdated
"previous_candidates": self.previous_candidates.tolist() | ||
if self.previous_candidates is not None | ||
else None, | ||
"random_states": [acq._serialize_random_state() for acq in self.base_acquisitions] |
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.
Are the random states for the base_acqs not already stored in Line 1212 under "base_acquisition_params"
?
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.
Each of these base acquisition functions in base_acquisitions have:
- A random state initialized inside of their acquisition function
- A high level random state from the BayesianOptimizer
The base_acquisition_params key holds the acquisition function parameters, which will include inside it 1. The random_states key holds the random state of the BayesianOptimizer random state, 2.
* chore: upgrade poetry2.0 & apply pep621 * fix: replace poetry action(not support 2.0) * fix: exclude one matrix * chore: split numpy deps * fix: numpy constraints * fix: install root * chore: use install-poetry
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #547 +/- ##
==========================================
+ Coverage 96.19% 96.43% +0.24%
==========================================
Files 10 12 +2
Lines 867 1235 +368
==========================================
+ Hits 834 1191 +357
- Misses 33 44 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hey all, long time listener first time contributor. I've created two new methods for the BayesianOptimizer class to allow users to save and load their optimizers while maintaining state. I've added a new section to the basic tour to demonstrate this cleaner method for saving and loading their optimizer.
A couple notes, users using a custom acquisition function will have to write their own get_acquisition_params and set_acquisition_params class functions in order to preserve exact state, otherwise there might be a loss of state when saving and loading. This should still be strictly better than the previous flow for saving and loading optimizers which ignored the acquisition parameters.
If this pull request gets merged into master, I'd like to remove the previous saving methodology, but I'll save that for a future PR.