**BREAKING**: makes GP evaluation explicitly require RowVecs/ColVecs wrapper; calling GP with AbstractMatrix now errors

[st--]

Resolves #257

[codecov]

Codecov Report

Merging #260 (f740a97) into master (ff3874f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #260   +/-   ##
=======================================
  Coverage   97.88%   97.89%           
=======================================
  Files          10       10           
  Lines         379      380    +1     
=======================================
+ Hits          371      372    +1     
  Misses          8        8           

Impacted Files	Coverage Δ
src/finite_gp_projection.jl	100.00% <100.00%> (ø)
src/sparse_approximations.jl	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff3874f...f740a97. Read the comment docs.

[st--]

Somehow the docs preview doesn't seem to reflect the changes.

The PPL test failure is unrelated (Turing/libtask_jll version incompatiblities).

[willtebbutt]

A couple of thoughts

[willtebbutt]

Oo, also -- could you add a small @test_throws test somewhere, to help ensure that we don't accidentally remove the informative error message at some point in the future?

[willtebbutt]

I'm happy with this now, but I would like @devmotion to approve, as he's been quite involved in the review of this PR.

[devmotion]

I think the PR is not completely ready yet but it's definitely better to decide and state clearly that matrices are not supported instead of allowing them but still printing a log message.

I have the following questions:

• Was there agreement that matrices should not be allowed in KernelFunctions either (I would be fine with it but I know that @theogf wanted to support matrices in the past)? I think this PR should only be merged after the behaviour in KernelFunctions is changed, and it should not be merged at all if we want to continue supporting matrices in KernelFunctions. It seems you also link to the documentation of KernelFunctions in the docstring but currently this documentation explains why matrices are supported: https://juliagaussianprocesses.github.io/KernelFunctions.jl/stable/design/#Why-We-Have-Support-for-Both
• Have you considered deprecating the constructor (possibly with a custom deprecation message) and removing it in the next breaking release instead of throwing an error? Throwing an error demands a breaking release but in this case we could just remove the constructor directly.
• Have you considered handling the matrices in (f::AbstractGP)(x...) instead of (or in addition) to the FiniteGP constructor? f(x...) seems to be the official API and hence it is a bit surprising to only error in an internal method - but to refer to f(x...) in the error message.

[devmotion]

These changes seem unrelated, would have been cleaner to make them in a separate PR.

[st--]

true. just done that

[theogf]

Was there agreement that matrices should not be allowed in KernelFunctions either (I would be fine with it but I know that @theogf wanted to support matrices in the past)? I think this PR should only be merged after the behaviour in KernelFunctions is changed, and it should not be merged at all if we want to continue supporting matrices in KernelFunctions.

I don't care anymore if we support matrices or not in KernelFunctions. I think this is a useful feature, but if we want to remove it it's fine. However, I disagree with the order of deprecation. We should deprecate AbstractGPs first as it has zero impact on KernelFunctions.jl. KF is still a very distinct part of the whole ecosystem anyway

[devmotion]

We should deprecate AbstractGPs first as it has zero impact on KernelFunctions.jl. KF is still a very distinct part of the whole ecosystem anyway

It's distinct but in both cases currently one can use ColVecs etc and matrices, so dropping support for matrices in only one of the packages is inconsistent. Both are part of the GP ecosystem and KernelFunctions defines and explains ColVecs etc. IMO the inconsistency becomes really apparent by the fact that the KernelFunctions docs - which are linked from the docstring in this PR - explicitly explain why matrices are supported as well.

As a middle ground, we could just remove the default value for obsdim (I mentioned this in a thread above). Eg Distances did the same, and this would avoid any confusion and implicit assumptions about whether rows or columns are observations.

[devmotion]

I think this PR should not be merged in light of the (soon to be merged) obsdim changes in KernelFunctions. Instead I think it would be more consistent to require an explicit obsdim argument as well for matrices.

[devmotion]

#264 was merged, so I think this PR can be closed.