-
Notifications
You must be signed in to change notification settings - Fork 1
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
adapt to new SurrogatesBase and improve performance of add_points #1
Conversation
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.
Thanks for your contributions, what version of SurrogatesBase are you referring to? Is it intentional to omit "at point" versions of mean
, var
, mean_and_var
, add_point!
?
kernel_creator::F | ||
end | ||
|
||
function Base.show(io::IO, ::MIME"text/plain", g::GPSurrogate) |
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 is cool, thanks!
gp::GP | ||
gp_posterior::GP_P | ||
hyperparameters::NamedTuple | ||
hyperparameters::H |
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.
👍
append!(g.xs, new_xs) | ||
append!(g.ys, new_ys) | ||
g.gp_posterior = AbstractGPs.posterior(AbstractGPs.FiniteGP(g.gp_posterior, | ||
new_xs, |
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.
In the posterior, we should use the whole history g.xs
, g.ys
, not just new_xs
, new_ys
.
The problem why we have to copy the history is that once we create a posterior, we cannot change the data. I think we cannot avoid copying the history each time we add new data, since we update the posterior each time.
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.
Now, I am a bit confused about the error.
The following does not throw an error:
function kernel_creator(hyperparameters)
return with_lengthscale(KernelFunctions.Matern52Kernel(), hyperparameters.lengthscale)
end
@testset "test" begin
xs= [1.0, 2.0, 3.0]
ys = [1., 2., 3.]
l = GPSurrogate(xs, ys, kernel_creator = kernel_creator,
hyperparameters = (; lengthscale = 1.0, noise_var = 0.1))
push!(l.xs, 10.)
push!(xs, 20.)
println(l.xs)
@show l.gp_posterior
end
But when I don't pass the keyword arguments,
@testset "test" begin
xs= [1.0, 2.0, 3.0]
ys = [1., 2., 3.]
l = GPSurrogate(xs, ys)
push!(l.xs, 10.)
push!(xs, 20.)
println(l.xs)
@show l.gp_posterior
end
it throws cannot resize array with shared data
.
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.
In the posterior, we should use the whole history
g.xs
,g.ys
I think the whole history is taken into account. Note that I'm updating the posterior, instead of recomputing the posterior from scratch, but this should be tested.
Copying the history and recomputing the posterior is insanely slow. If a fast alternative is not possible with AbstractGPs, we modify AbstractGPs or we use GaussianProcesses.jl, where I use the fast ElasticPDMats.
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.
Alright thanks! I found out in the AbstracGPs docs that it is possible to do squential conditioning. (Available here)
y_copy) | ||
g.xs, g.ys, g.gp_posterior = x_copy, y_copy, updated_posterior | ||
return nothing | ||
GPSurrogate(copy(xs), |
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.
Would it be better to use copied input in the surrogate? It is probably safer to do so, avoiding mutations from outside. 👍
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 is just for the construction. We can also have copying optional with a keyword argument.
|
I merged all proposed changes while adapting to recent changes of SurrogatesBase. Copying of the arguments is not yet optional though. |
No description provided.