-
Notifications
You must be signed in to change notification settings - Fork 36
Adds kernel related to the Intrinsic Coregionalization Model #263
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
Adds kernel related to the Intrinsic Coregionalization Model #263
Conversation
parameterization of the coregionalization matrix. Removes white space. Renames file. Updates names in module file. Corrects check_args. Corrects using incorrect field name in coregion covariance function. Cleans white space.
So to test AD we have a KernelFunctions.jl/test/test_utils.jl Line 54 in 0867f29
In your case this would be something like: test_ADs(L -> CoRegionKernel(SqExponentialKernel(), L * L'), rand(2, 2)) One could also add |
Makes parameter explicit in function definition. Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
I think that function cannot be applied here because the random inputs it generates aren't designed for Multi output kernels. KernelFunctions.jl/test/test_utils.jl Line 82 in 0867f29
|
I also think that the using KernelFunctions
using FiniteDifferences
A = randn(2,2)
B = A * A'
k = CoregionMOKernel(ConstantKernel(), B)
FDM = FiniteDifferences.central_fdm(5, 1)
x = rand(2)
y = (rand(2), 1) julia> FiniteDifferences.grad(FDM, x -> k((x,1), y), x)
([1.7763042552775138e-14, 1.7763042552775138e-14],) Otherwise we would get an error ERROR: MethodError: no method matching (::CoregionMOKernel{ConstantKernel{Float64},Array{Float64,2}})(::Tuple{Array{Float64,1},Float64}, ::Tuple{Array{Float64,1},Int64}) because |
Oh you're right, we probably need a different solution then... |
We shouldn't need a separate suite of tests -- we should just need to generate inputs that are appropriate for multi-output kernels. Should just need an additional method around here, and then just run the standard suite of tests with that input. |
Now I'm confused on how does AD work for MO kernels in a general setting, i.e, if we have to adapt the inputs for it to work in our tests how will it work when the inputs are not adapted? |
Should we create a new type for the inputs of Multioutput kernels, and then create a new method for Such type could be called |
Is there a problem with just using a |
Yeah I was reading the documentation now and realized that too. I guess I'll try to implement that. Thanks. |
I added methods to some functions that are owned by After loading the code, running: fdm = central_fdm(5, 1)
x1 = (1.0, 1)
x2 = (2.0, 1)
A = [2 1; 1 3]
kernel = ExponentialKernel()
coregion = CoregionMOKernel(kernel, A)
grad(fdm, coregion, x1, x2) outputs: (0.735758882342766, -0.7357588823428793) While Zygote.gradient(coregion, x1, x2) outputs: ((0.7357588823428847, nothing), (-0.7357588823428847, nothing)) |
If this gets accepted we can improve this test afterwards. |
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 looks really close to being ready to go. Just a couple of things.
@4aHxKzD sorry, I missed your last comment before reviewing. I think the main point here is that we really want to ignore the |
Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
I think that this is done guys. |
I'm happy -- I'll leave @devmotion to review. Looks like there are some formatting things to sort though. |
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.
Looks good, apart from the constructor and the format issues. Can you also update the version number in Project.toml?
Thanks for the contribution @4aHxKzD and working through the pain points. Our tools for implementing / testing multi-output kernels aren't very refined yet, so it's been really helpful to have someone work through the implementation and clarify what needs work etc. |
Thank you 😄 I learned a lot of Julia with you guys. |
As requested and discussed in #128
I don't know how to work with the AD packages, so I didn't add tests to cover that. Can you guide me on this please?