Testing (and fixing) handling of AbstractVector{AbstractVector{T}} inputs

[Crown421]

Summary While implementing stuff for #363 I noticed issues with the handling of AbstractVector{AbstractVector{T}} as inputs for kernelmatrix. Specifically, I noticed a missing method for the fbmkernel, and started by adding tests for AbstractVector{AbstractVector{T}} to the test interface. This has spawned additional issues.

Proposed changes

• Adding tests for AbstractVector{AbstractVector{T}} inputs
• Possibly fixing uncovered issues with, among others, the delta metric, _mod in fbm kernel.

What alternatives have you considered? I understand that AbstractVector{AbstractVector{T}} inputs are not recommended, and that RowVecs and ColVecs should be used instead. However, in the current state those inputs work for some kernels, but not for others. I see two alternatives

1. Disallow AbstractVector{AbstractVector{T}} inputs, i.e. via kernelmatrix(k, AbstractVector{AbstractVector{T}}, AbstractVector{AbstractVector{T}}) = error("....., see documentation for details")
2. Go through with this PR, and add a maxlog=1 warning that vectors of vectors are not recommended.

Breaking changes Current state none, alternative 1 will probably be breaking.

[willtebbutt]

Vey much in favour of ensuring that this works -- if a kernel says its inputs are vector-valued then kernelmatrix ought to work with any AbstractVector{<:AbstractVector} of inputs.

At what point should we review this PR properly? It looks like it's got some stuff for vector-of-vectors collections of inputs, but also some fixes for kronecker-structured covariance matrices. Is the intention that they be reviewed together, or done separately?

[theogf]

I understand that AbstractVector{AbstractVector{T}} inputs are not recommended

I don't think this is true at all, these inputs should work just as well!

[Crown421]

At what point should we review this PR properly? It looks like it's got some stuff for vector-of-vectors collections of inputs, but also some fixes for kronecker-structured covariance matrices. Is the intention that they be reviewed together, or done separately?

Apologies for this, all but the last commit are actually part of #369 , but I did not pay attention when branching and don't know enough about git to fix it. #369 should be ready to be reviewed.

For this commit, given that the expression towards fixing the issues, I will add them here "soon". I have solutions for the issues I found so far, but it is of course possible that more comes up.

[willtebbutt]

No problem at all. Lets get 369 sort, then turn our attention back to this PR.

[Crown421]

I understand that AbstractVector{AbstractVector{T}} inputs are not recommended

I don't think this is true at all, these inputs should work just as well!

The section that I am referring to is "We recommend that collections of vector-valued inputs are stored in an AbstractMatrix{<:Real} when possible," in the API section

[willtebbutt]

@Crown421 is this the PR that you'd like eyes on next?

[Crown421]

@Crown421 is this the PR that you'd like eyes on next?

I think the answer is yes. I fixed the two issues that revealed themselves and extended tests pass again.

[devmotion]

Sorry, I missed this PR. I have some doubts about the changes to Delta, can we fix this in some other way by e.g. defining (::Delta)(x::AbstractVector{<:AbstractVector}, y::AbstractVector{<:AbstractVector}) instead (I assume the problem are vector of vectors as inputs, given the title of the PR)?