-
-
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
Make GEKPLS differentiable and surrogate optimization compatible #381
Make GEKPLS differentiable and surrogate optimization compatible #381
Conversation
…vior to other surrogates
Codecov Report
@@ Coverage Diff @@
## master #381 +/- ##
==========================================
+ Coverage 79.18% 79.65% +0.47%
==========================================
Files 16 16
Lines 2306 2335 +29
==========================================
+ Hits 1826 1860 +34
+ Misses 480 475 -5
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
src/GEKPLS.jl
Outdated
if size(y, 1) > 1 | ||
return vec(y) | ||
else | ||
return y[1] #this is necessary for differentiation; Zygote expects a scalar output |
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 will be type-unstable. It's not necessary for differentiation if one uses a closure.
But it should match the other surrogates. What do they do here?
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.
The other surrogates assume that input will be a single record (be it a tuple or vector). GEKPLS also now does the same (since it now includes the _check_dimension(g, x_vec)
function. That if
condition is unnecessary.
I'll update the code. Thanks :)
Co-authored-by: Christopher Rackauckas <[email protected]>
The optimization check fails because maxiters and num_new_samples which are, by default set to 100 each have been reduced. So I have increased tolerance, maxiters and num_new_samples.
Makes it more convenient for users to add lower and upper bounds without having to concatenate before sending in params to GEKPLS constructor.
Awesome!!!! 🎉
Yes it's a bit non-trivial, but you can use |
Following are the changes made:
1. GEKPLS now accept inputs like other surrogates
The earlier version of GEKPLS had inputs that required matrices. This was in response to issue #355 and I had hoped that it would be relatively straight-forward to convert all surrogates to accept matrices as inputs. However, in trying to integrate with src/optimization.jl, I discovered that this may be way more involved than I'd originally imagined. Hence, I have changed the input format for GEKPLS. It now accepts vectors of tuples as inputs and tuples for predicting a point estimate.
2. GEKPLS is now differentiable
So, if we have a GEKPLS object called 'g', we can do something like
Zygote.gradient(g, (1.0, 1.0, 1.0))
(issue #371)3. GEKPLS now plays well with optimization.jl from surrogates
So we can do something like:
surrogate_optimize(sphere_function, SRBF(), lb, ub, g, UniformSample(); needs_gradient = true)
Note the new flag I've added called '
needs_gradient
'. The idea is that as we add other gradient enhanced methods like GENN, we can set this flag to true as needed. It does not affect the behavior or performance of any of the other surrogates as it is set to false by default.4. Added doc strings
Key functions now have doc strings. Fixes #373.
5. Made struct more generic
Partially addresses #364