KernelFunctions.jl icon indicating copy to clipboard operation
KernelFunctions.jl copied to clipboard

Make ARDTransform a subtype of LinearTransform?

Open devmotion opened this issue 5 years ago • 4 comments

As discussed in https://github.com/JuliaGaussianProcesses/KernelFunctions.jl/pull/106, the ARDTransform could be viewed as a special LinearTransform with a diagonal matrix. Do we still want to keep these separate types?

devmotion avatar Apr 27 '20 13:04 devmotion

I would strongly push for keeping ARDTransform separate from LinearTransform. It is indeed a special case but it is a very used one. In the same sense that SqExponentialKernel is just a special case of the GammaSqExponentialKernel

theogf avatar Apr 27 '20 13:04 theogf

I agree that users should have easy access an ARD transform, but I think it's very debatable whether or not this means we need to maintain an entirely separate transform from the LinearTransform unless there are good performance reasons to do so.

We could just alias const ARDTransform = LinearTransform{<:Diagonal} (or whatever the correct parametrisation is) and no longer have to maintain the corresponding code as a consequence.

For me at least, the difference between this and the SqExponentialKernel case is that here whether or not something is a ARD is determined by the type of its argument, so we know everything at compile time and can specialise appropriately using dispatch.

willtebbutt avatar Apr 27 '20 13:04 willtebbutt

I agree. IMO there's no particular reason for not unifying the underlying implementation (which a user shouldn't deal with or be concerned about). As @willtebbutt says, one can introduce an alias and still provide constructors for vectors to users, even if one makes ARDTransform a subtype of LinearTransform.

devmotion avatar Apr 27 '20 20:04 devmotion

I am not sure I see the point of it but okay...

theogf avatar Apr 28 '20 08:04 theogf