Deprecate implicit obsdim keyword argument

[devmotion]

Lately, due to the discussions also in AbstractGPs, I thought a bit about our support of matrices and the obsdim keyword argument.

I realized that I like it that one can still call kernelmatrix etc. with matrices, even though internally we only work with RowVecs and ColVecs (fortunately 🙏). I think this might make it a bit easier for new users as they don't have to learn about ColVecs and RowVecs right away - and even to me sometimes it feels quite nice, e.g., if other parts and packages of my script don't work with, or don't even know about, ColVecs/RowVecs and the hence the most natural form of the data is a raw matrix with a clearly defined convention (rows or columns).

However, I also realized that I really don't like it that there is an implicit default obsdim since this makes code more difficult to read and reason about and could lead to misunderstandings and bugs. And any default choice is somewhat arbitrary and probably heavily biased by research area and personal preferences.

I think one can get both - matrices as arguments and no default convention - by deprecating the default implicit obsdim keyword argument and requiring users to state obsdim explicitly. A prominent example of another package that did this is Distances.jl.

The idea grew on me, and hence I put together this PR. It has to be updated once #427 is merged.

[codecov]

Codecov Report

Merging #429 (455df6e) into master (f9bbd84) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #429      +/-   ##
==========================================
+ Coverage   93.09%   93.14%   +0.04%     
==========================================
  Files          52       52              
  Lines        1202     1210       +8     
==========================================
+ Hits         1119     1127       +8     
  Misses         83       83              

Impacted Files	Coverage Δ
src/approximations/nystrom.jl	92.50% <100.00%> (ø)
src/matrix/kernelmatrix.jl	100.00% <100.00%> (ø)
src/matrix/kernelpdmat.jl	81.81% <100.00%> (ø)
src/utils.jl	91.13% <100.00%> (+0.99%)	⬆️

---

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 f9bbd84...455df6e. Read the comment docs.

[theogf]

So the idea behind the deprecation is that defaultobs is now nothing and that it will trigger an warning but still work like before and we then make a new breaking release without this default?

[devmotion]

Yes, exactly, currently it is just changing defaultobsdim to nothing, widening the type annotation, and adding a deprecation warning if obsdim=nothing. In an upcoming breaking release we would then remove defaultobsdim and change obsdim::Union{Int,Nothing}=defaultobsdim to just obsdim::Int, without default value.

[theogf]

Can you maybe comment or introduce what will be the behavior when the user will not provide obsdim (once the deprecations are removed)

[devmotion]

The default behaviour would be:

julia> f(x; obsdim::Int) = size(x, obsdim)
f (generic function with 1 method)

julia> f(rand(3, 4))
ERROR: UndefKeywordError: keyword argument obsdim not assigned
Stacktrace:
 [1] f(x::Matrix{Float64})
   @ Main ./REPL[6]:1
 [2] top-level scope
   @ REPL[7]:1

julia> f(rand(3, 4); obsdim=nothing)
ERROR: TypeError: in keyword argument obsdim, expected Int64, got a value of type Nothing
Stacktrace:
 [1] top-level scope
   @ REPL[8]:1

julia> f(rand(3, 4); obsdim=1)
3

julia> f(rand(3, 4); obsdim=2)
4

If we are not happy with these messages, we could continue using nothing but throw an error in vec_of_vecs where in this PR a deprecation warning is shown.

[theogf]

Yes that's exactly what we discussed last time with @st--, @willtebbutt and @rossviljoen . We think that it should throw an error but with a extremely verbose explicative message

[st--]

what was your reason to here use an @assert instead of an explicit if & throw(ArgumentError())?

[devmotion]

That I didn't want to change the existing code 😄 I think it should throw an error but this seemed unrelated to the changes here.

[devmotion]

Since I prefer ArgumentError as well I changed it nevertheless.

[st--]

Looks great, thanks! Happy to have this merged into master.

[st--]

What's the benefit of the ternary ? : over just

Suggested change

	) sampleindex(vec_of_vecs(X; obsdim=obsdim === nothing ? nothing : Int(obsdim)), r) false
	) sampleindex(vec_of_vecs(X; obsdim=obsdim), r) false

?

[st--]

is it because here it was Integer whereas vec_of_vecs requires more specific Int?

[devmotion]

Yes, exactly. The current deprecation is not correct since it does not handle non-Int Integers. It doesn't matter much since we're allowed to remove it any time but since I had to change it anyway I fixed it.