Testing (and fixing) handling of AbstractVector{AbstractVector{T}} inputs #370
Conversation
|
Vey much in favour of ensuring that this works -- if a kernel says its inputs are vector-valued then 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? |
I don't think this is true at all, these inputs should work just as well! |
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. |
|
No problem at all. Lets get 369 sort, then turn our attention back to this PR. |
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 |
|
@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. |
| @@ -133,6 +133,18 @@ function test_interface( | |||
| ) | |||
| end | |||
|
|
|||
| function test_interface( | |||
| rng::AbstractRNG, k::Kernel, ::Type{<:Vector{Vector{T}}}; dim_in=2, kwargs... | |||
There was a problem hiding this comment.
| rng::AbstractRNG, k::Kernel, ::Type{<:Vector{Vector{T}}}; dim_in=2, kwargs... | |
| rng::AbstractRNG, k::Kernel, ::Type{<:AbstractVector{<:AbstractVector{T}}}; dim_in=2, kwargs... |
Sorry what's the reasoning for not using this?
There was a problem hiding this comment.
The test itself utilises a Vector{Vector{T}}, so to my mind this type constraint better reflects what's going on 🤷
| @inline Distances.eval_op(::Delta, a::Real, b::Real) = a == b | ||
| @inline Distances.eval_reduce(::Delta, a, b) = a && b | ||
| @inline Distances.eval_start(::Delta, a, b) = true |
There was a problem hiding this comment.
This will result in much more complicated code, I don't think we should use the default dispatches in Distances and implement these internal functions.
There was a problem hiding this comment.
We should just have to check x == y for the inputs x and y.
There was a problem hiding this comment.
Is there even a reason to implement Distances._evaluate here? Isn't (::Delta)(x, y) sufficient?
|
Sorry, I missed this PR. I have some doubts about the changes to |
Summary
While implementing stuff for #363 I noticed issues with the handling of
AbstractVector{AbstractVector{T}}as inputs forkernelmatrix. Specifically, I noticed a missing method for thefbmkernel, and started by adding tests forAbstractVector{AbstractVector{T}}to the test interface. This has spawned additional issues.Proposed changes
AbstractVector{AbstractVector{T}}inputsdeltametric,_modinfbmkernel.What alternatives have you considered?
I understand that
AbstractVector{AbstractVector{T}}inputs are not recommended, and thatRowVecsandColVecsshould be used instead. However, in the current state those inputs work for some kernels, but not for others. I see two alternativesAbstractVector{AbstractVector{T}}inputs, i.e. viakernelmatrix(k, AbstractVector{AbstractVector{T}}, AbstractVector{AbstractVector{T}}) = error("....., see documentation for details")maxlog=1warning that vectors of vectors are not recommended.Breaking changes
Current state none, alternative 1 will probably be breaking.