Skip to content

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

Merged
merged 67 commits into from
Jun 2, 2021
Merged

Adds kernel related to the Intrinsic Coregionalization Model #263

merged 67 commits into from
Jun 2, 2021

Conversation

david-vicente
Copy link
Contributor

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?

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.
@theogf
Copy link
Member

theogf commented Mar 22, 2021

So to test AD we have a test_ADs function which takes a function creating the kernel given some arguments :

function test_ADs(

In your case this would be something like:

test_ADs(L -> CoRegionKernel(SqExponentialKernel(), L * L'), rand(2, 2))

One could also add B as a training parameter using @functor CoregionMOKernel but I don't think it's a good idea since B needs to be PSD.

@david-vicente
Copy link
Contributor Author

So to test AD we have a test_ADs function which takes a function creating the kernel given some arguments :

function test_ADs(

In your case this would be something like:

test_ADs(L -> CoRegionKernel(SqExponentialKernel(), L * L'), rand(2, 2))

I think that function cannot be applied here because the random inputs it generates aren't designed for Multi output kernels.

x = rand(rng, dims[1])

@david-vicente
Copy link
Contributor Author

david-vicente commented Mar 23, 2021

I also think that the Int part of the input tuple is a problem for the implemented tests. We would have to change the code so that only the location is variable and the output dimension is fixed when testing the gradient of the covariance functions.

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 FiniteDifferences.to_vec changes the Int part of the tuple to a Float.

@theogf
Copy link
Member

theogf commented Mar 23, 2021

I think that function cannot be applied here because the random inputs it generates aren't designed for Multi output kernels.

Oh you're right, we probably need a different solution then...
It's probably worth it to have a different suite of tests for MO kernels. @willtebbutt @devmotion ?

@willtebbutt
Copy link
Member

willtebbutt commented Mar 23, 2021

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.

@david-vicente
Copy link
Contributor Author

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?

@david-vicente
Copy link
Contributor Author

david-vicente commented May 25, 2021

Should we create a new type for the inputs of Multioutput kernels, and then create a new method for FiniteDifferences.to_vec to dispatch on, as well as new ChainRules rules for this type of kernels?

Such type could be called MOInput, and the current MOInput could be renamed to something like AllObservedMOInput or FullyObservedMOInput.

@willtebbutt
Copy link
Member

Such type could be called MOInput, and the current MOInput could be renamed to something like AllObservedMOInput or FullyObservedMOInput.

Is there a problem with just using a Vector{Tuple{T, Int}} in cases where not every output is observed all of the time?

@david-vicente
Copy link
Contributor Author

david-vicente commented May 25, 2021

Is there a problem with just using a Vector{Tuple{T, Int}} in cases where not every output is observed all of the time?

Yeah I was reading the documentation now and realized that too. I guess I'll try to implement that. Thanks.
Do you have a particular place where I should put that code?

@david-vicente
Copy link
Contributor Author

david-vicente commented May 27, 2021

I added methods to some functions that are owned by FiniteDifferences.jl. I didn't know exactly where to put them so ended up placing them in the test directory because I believe those are only going to be used for testing real AD code against to. Would you be kind to review this?
I'm not completely sure about the Type annotations I used in the signatures.
I also don't really know which file should call this file I'm adding. Maybe KernelFunctions.jl/test/runtests.jl ?

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))

@david-vicente
Copy link
Contributor Author

If this gets accepted we can improve this test afterwards.

Copy link
Member

@willtebbutt willtebbutt left a 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.

@willtebbutt
Copy link
Member

@4aHxKzD sorry, I missed your last comment before reviewing.

I think the main point here is that we really want to ignore the Int output dimension-specifiers in the input dimensions. My suggestions for a to_vec method should sort this out.

@david-vicente
Copy link
Contributor Author

I think that this is done guys.

@willtebbutt
Copy link
Member

I'm happy -- I'll leave @devmotion to review.

Looks like there are some formatting things to sort though.

Copy link
Member

@devmotion devmotion left a 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?

@willtebbutt
Copy link
Member

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.

@willtebbutt willtebbutt merged commit 4569859 into JuliaGaussianProcesses:master Jun 2, 2021
@david-vicente
Copy link
Contributor Author

Thank you 😄 I learned a lot of Julia with you guys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants