-
-
Notifications
You must be signed in to change notification settings - Fork 70
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
Parallel Surrogate Models Through Ask-tell Interface #435
Conversation
Hey, this is really cool and it's sad to see it just live as a draft. Is there an intention to complete this? |
Absolutely! I apologize that I haven't finished this; I honestly completely forgot about it. I'll probably have some time to do some more this weekend. |
Sorry for the delay, but I think the PR is finally ready to look over. Please let me know your thoughts if you have the time and I'd be happy to make the necessary changes. |
dtol = 1e-3 * norm(ub - lb) | ||
eps = 0.01 | ||
|
||
tmp_krig = deepcopy(krig) # Temporary copy of the kriging model to store virtual 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.
This can be pretty heavy. Did you check it did not cause a significant performance issue?
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.
I did some tests and it seems like in all cases, deepcopy
will never be anywhere close to your bottleneck. The bottleneck is always either evaluating your objective function or calculating the expected improvement or merit function.
Overall it looks good, but just needs a few bikeshedding tweaks in order to be ready to merge. We need to be careful that things specialize and exporting 2-3 letter words is generally not a good idea for something that's widely used, so we need to just change a few surface level things. |
I think I was able to address your concerns. Is there anything else you'd like me to change? |
my_k = Kriging(x, y, lb, ub) | ||
|
||
for _ in 1:10 | ||
new_x, eis = potential_optimal_points(EI(), lb, ub, my_k, SobolSample(), 3, CLmean!) |
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.
new_x, eis = potential_optimal_points(EI(), lb, ub, my_k, SobolSample(), 3, CLmean!) | |
new_x, eis = potential_optimal_points(EI(), lb, ub, my_k, SobolSample(), 3, MeanConstantLiar()) |
?
I think this looks great! It may need to wait for #441 to fix tests on master and rebase on top of that, but I think this is looking ready to merge if tests pass. |
4edf555
to
ea4eef2
Compare
Codecov Report
@@ Coverage Diff @@
## master #435 +/- ##
==========================================
- Coverage 73.68% 65.58% -8.11%
==========================================
Files 22 23 +1
Lines 2930 3115 +185
==========================================
- Hits 2159 2043 -116
- Misses 771 1072 +301
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
🎉 |
As mentioned in #278, it would be nice to be able to be able to have surrogate models ask for multiple points at once to enable parallelized surrogate modeling. This PR introduces an
Ask
function that a user can supply a surrogate model and a set number of points. To maximize the expected improvement of a given model in parallel, temporary virtual values will have to be assigned sequentially for each parallel point. I have implemented 6 different methods of getting virtual points. These areMore information about these virtual point methods can be found in the documentation for the surrogate modeling toolbox and this paper that Scikit Optimize references.
Below is an example of how
Ask
could be used. The functionadd_point
, works as a "tell", so you can have an Ask-tell interface.In its current state, this PR add an
Ask
that only works for Kriging models with an EI acquisition function. I also have not added added documentation yet. I plan to add docs and I would be willing to get this to work for more surrogate model types of acquisition functions, I would just like to get a confirmation that this looks good so far and that I'm on the right track. If there are any suggestions for improvements or ways to other features to add to this PR, I would be happy to hear them.Also mentioned in #278 was that some samplers won't work with parallel sampling. Like it was said there, I think it would make sense to add a type restriction to
Ask
so that only parallelizable samplers work. Is there any way I could add a subtype of the samplers in this repository or would that change have to be made in QuasiMonteCarlo.jl?