**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

❌ Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 0.25%. Comparing base (8b49ef6) to head (3f9897e).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
src/finite_gp_projection.jl	0.00%	3 Missing ⚠️
src/util/ad.jl	0.00%	3 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (8b49ef6) and HEAD (3f9897e). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (8b49ef6)	HEAD (3f9897e)
	2	1

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #306       +/-   ##
==========================================
- Coverage   96.21%   0.25%   -95.96%     
==========================================
  Files          12      13        +1     
  Lines         396     387        -9     
==========================================
- Hits          381       1      -380     
- Misses         15     386      +371     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[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