Length check before applying TensorProduct

[sharanry]

As discussed with @willtebbutt earlier, TensorProduct doesn't check the dimension of the input before applying. This can lead to bugs which are hard to trace.

Currently:

julia> k = TensorProduct(ConstantKernel(c=2.0), ConstantKernel(c=2.0))
Tensor product of 2 kernels:
	- Constant Kernel (c = 2.0)
	- Constant Kernel (c = 2.0)

julia> k(rand(3), rand(3))
4.0

It picks up the first length(k) dimensions of the input, ignoring the rest.

[willtebbutt]

Thanks for opening an issue to sort this out. I think a slightly more general solution is in order though, as this problem is definitely floating around all over the place, and our errors are not the most easy to decipher at the minute.

In particular, the validate_dims and validate_inplace_dims functions in util.jl should be extended to also accept the kernel as an argument, and a check added to ensure that the inputs and the kernel have consistent input dimensionality -- this could be done cleanly by adding methods of dim for a various kernels.

Not all kernels have a fixed / known input dimensionality, so a sensible fallback for dim(::Kernel) would be a singleton type called ArbitraryDimensionality with no fields that you can error check on. I'm thinking something like

struct ArbitraryDimensionality end

dim(::Kernel) = ArbitraryDimensionality()
dim(k::TensorProduct) = length(kernel.kernels)

# Example validate_dims implementation

function validate_dims(k::Kernel, x::AbstractVector, y::AbstractVector)
    if dim(x) != dim(y)
        throw(DimensionMismatch(
            "Dimensionality of x ($(dim(x))) not equality to that of y ($(dim(y)))",
        ))
    end
    validate_kernel_dims(k, x)
end

function validate_kernel_dims(k::Kernel, x::AbstractVector)
    if dim(k) !== ArbitraryDimensionality() && dim(k) != dim(x)
        throw(DimensionMismatch(
            "Dimensionality of kernel ($(dim(k))) not compatible ",
            "with that of input ($(dim(y)))",
        ))
    end
end

If you would rather open this out into a separate issue, that's also fine. But I think my comment regarding the type of error should definitely be addressed before merging as is.

[devmotion]

Not all kernels have a fixed / known input dimensionality, so a sensible fallback for dim(::Kernel) would be a singleton type called ArbitraryDimensionality with no fields that you can error check on. I'm thinking something like

An alternative would be to define

isdimcompatible(::Kernel, x) = true
isdimcompatible(kernel::TensorProductKernel, x) = length(kernel.kernels) == dim(x)

function validate_kernel_dims(k::Kernel, x::AbstractVector)
    if isdimcompatible(k, x)
        throw(DimensionMismatch(
            "Dimensionality of kernel not compatible ",
            "with that of input",
        ))
    end
end

and only define isdimcompatible(kernel, x) if needed.

[willtebbutt]

Think I would prefer the name dims_are_compatible(k, x), but that's a good point @devmotion .

[theogf]

Actually do you think this is something we could put in common to use with specific transforms. Like if one use LinearTransform or ARDTransform the dimension of the kernel is no more arbitrary.

[devmotion]

I guess you could just define (not sure about the exact fields an types)

function dims_are_compatible(kernel::TransformedKernel{<:Kernel,<:LinearTransform}, x)
    return size(kernel.transform.P, 2) == dim(x)
end

Did you have that in mind?

[theogf]

Yeah but it gets trickier for more complex structures. I think it's better to leave the error happen when the transformation is applied, and return a meaningful message.

[sharanry]

If you would rather open this out into a separate issue, that's also fine. But I think my comment regarding the type of error should definitely be addressed before merging as is.

@willtebbutt I have addressed this suggestion. I will create a separate PR for a more general solution?

[willtebbutt]

LGTM. Will merge once tests pass.

[willtebbutt]

I will create a separate PR for a more general solution?

@sharanry yes please 🙂