**BREAKING** improve structs & constructors

[st--]

Happy for others to contribute to this branch.

Things that came to my mind:
e.g. FiniteGP should check whether length(x) == size(Sigma, 1) == size(Sigma, 2).

[willtebbutt]

e.g. FiniteGP should check whether length(x) == size(Sigma, 1) == size(Sigma, 2).

This would be good. We should be it in an @non_differentiable function though, for AD's sake. (we already have the ChainRulesCore dep, so no harm done there)

[codecov]

Codecov Report

Merging #306 (ac78254) into master (0440ea6) will increase coverage by 0.27%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #306      +/-   ##
==========================================
+ Coverage   97.64%   97.91%   +0.27%     
==========================================
  Files          10       10              
  Lines         382      384       +2     
==========================================
+ Hits          373      376       +3     
+ Misses          9        8       -1     

Impacted Files	Coverage Δ
src/finite_gp_projection.jl	100.00% <100.00%> (ø)
src/util/common_covmat_ops.jl	97.61% <0.00%> (+2.38%)	⬆️

---

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 0440ea6...ac78254. Read the comment docs.

[devmotion]

I'm fine with the type restriction but I still think it should go into a breaking release since it is not only theoretically breaking. Copied from #236 (comment)_:

This is not correct. Code written for the FiniteGP type does not support UniformScaling scalars / vectors as the observation noise covariance. If such types are passed, outer constructors are used to convert them into appropriate AbstractMatrix objects. The lack of a type constraint is an omission, and therefore a bug. For example, if you take a look through this code, you'll see that there are a lot of things that are done that are only safe to do with AbstractMatrixs. For example this.

I just checked and UniformScaling is supported and can be used at least with some methods (mean_and_var errors since diagind is not defined):

julia> using AbstractGPs, LinearAlgebra


julia> fx = GP(GaussianKernel())(rand(10), 2.0*I)
AbstractGPs.FiniteGP{GP{AbstractGPs.ZeroMean{Float64}, SqExponentialKernel{Distances.Euclidean}}, Vector{Float64}, UniformScaling{Float64}}(
f: GP{AbstractGPs.ZeroMean{Float64}, SqExponentialKernel{Distances.Euclidean}}(AbstractGPs.ZeroMean{Float64}(), Squared Exponential Kernel (metric = Distances.Euclidean(0.0)))
x: [0.9001122128843618, 0.5014532689115688, 0.9068485710635356, 0.8671710704251859, 0.1593165289491113, 0.033849226130907684, 0.6012613832007633, 0.6792056597108606, 0.6291170664768985, 0.4586633196418336]
Σy: UniformScaling{Float64}(2.0)
)


julia> mean(fx)
10-element Vector{Float64}:
 0.0
 0.0
 0.0
 0.0
 0.0
 0.0
 0.0
 0.0
 0.0
 0.0

julia> mean_and_cov(fx)
([0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0], [3.0 0.9236108289936744 … 0.9639467883634513 0.9071580732113963; 0.9236108289936744 3.0 … 0.9918840906723075 0.9990849290537535; … ; 0.9639467883634513 0.9918840906723075 … 3.0 0.9855777713218073; 0.9071580732113963 0.9990849290537535 … 0.9855777713218073 3.0])

julia> rand(fx)
10-element Vector{Float64}:
  3.7182207380247623
  1.0497036329028469
  0.9905553822300871
 -0.44238892519088857
 -1.0465937939874175
 -0.8895511203455914
  1.0418168460349009
 -0.20439667340116374
  1.2400217065941217
  0.12529989634261757

[st--]

I just checked and UniformScaling is supported and can be used at least with some methods (mean_and_var errors since diagind is not defined):

Could we not make that non-breaking by also having a constructor that converts the UniformScaling into an appropriately-sized ScalMat?

[willtebbutt]

@devmotion I'm still not entirely convinced by the UniformScaling argument. For me, it's still appropriate to consider it a bug, because if I ran the standard test_finitegp_primary_public_interface test suite on a FiniteGP with a UniformScaling in it, it would error, meaning that our "official" interface tests fail when you use a UniformScaling. Moreover, in the docs we explicitly say

Let f be an AbstractGP, x an AbstractVector representing a collection of inputs, and Σ a positive-definite matrix of size (length(x), length(x)). 

The docstring for FiniteGP is nearly as specific:

The finite-dimensional projection of the AbstractGP `f` at `x`. Assumed to be observed under
Gaussian noise with zero mean and covariance matrix `Σy`

although it should probably be tightened up, and docstrings added for the other outer constructors.

In short, our official documentation, to some degree the docstring for FiniteGP, and our official API tests indicate that Σy should be a matrix. To my mind, the fact that the implementations of a subset of the methods implemented on FiniteGPs happen to work correctly when a UniformScaling is provided is a coincidence, and not a sufficient reason to say that meaningfully support UniformScaling at present.

[st--]

Could we not make that non-breaking by also having a constructor that converts the UniformScaling into an appropriately-sized ScalMat?

Looks like we can, @devmotion I added the example you gave as another test case and it's passing

[devmotion]

A constructor won't prevent breakage of downstream dispatches on UniformScaling. If we consider this a valid example (maybe there are others, it was just the first thing that came to my mind and worked for large parts of the API, ie users might not notice that it was unintended), then there is no way around making a breaking release.

[devmotion]

@willtebbutt I can definitely see your point. IMO it's an edge case and one could argue one way or the other. I don't think the use of UniformScaling is completely surprising since eg MvNormal happily accepts these as covariance matrices.

In general, I think the discussion just shows that it would be safer to make a breaking release. Not all matrix-like structures are of type AbstractMatrix, as eg UniformScaling and up until some time ago AbstractPDMat show. And I don't think there's a major issue for downstream packages and users with making a beeaking release: if they don't use non-AbstractMatrix types they can just update, and otherwise it's good we didn't break stuff.

[willtebbutt]

Okay. Let's try and tie this down in the next release though (more precise testing and documentation).

[st--]

See JuliaGaussianProcesses/ApproximateGPs.jl#126 for the required AD fix