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

Remove type aliases for `ExpLink` etc. and/or the constructors?

Open devmotion opened this issue 4 years ago • 1 comments

This issue, or rather discussion of the desired API, came up in https://github.com/JuliaGaussianProcesses/GPLikelihoods.jl/pull/48.

With this PR now ExpLink etc. is a type alias of Link{typeof(exp)} and ExpLink() calls Link(exp). Intentionally, the PR was non-breaking. However, it leads to the following questions:

  1. Should we deprecate the type aliases and remove them eventually?
  2. Should we deprecate the constructors and remove them eventually?

Personally, I am favour of 2 and probably of 1:

  • I don't see a clear advantage of eg. ExpLink over Link(exp) but there would be fewer definitions and the API would be cleaner if the former would be removed.
  • It is less convenient to dispatch on Link{typeof(exp)} instead of ExpLink. However, I think it is questionable if this use case is a strong enough argument to include type aliases in GPLikelihoods. Instead in my opinion it would be sufficient if users and downstream packages define (possibly internal) type aliases if it is useful for their implementation. So currently I think type aliases should only be included but not necessarily exported if they are useful and used in GPLikelihoods.

@theogf suggested to add a macro @link CosLink cos that would define both the type alias and constructor (hence it assumes that we don't deprecate any of them). I'm not in favour of a macro, both because I think we should deprecate probably both definitions and because I think the definitions are already quite short and simple and so the macro feels too magical given these limited benefits.

devmotion avatar Oct 08 '21 21:10 devmotion

An additional argument for removing the type aliases and constructors is https://github.com/JuliaGaussianProcesses/GPLikelihoods.jl/pull/51: It shows that (it seems) one could remove Link (and also AbstractLink) completely, as suggested by @st-- in https://github.com/JuliaGaussianProcesses/GPLikelihoods.jl/issues/43.

devmotion avatar Oct 09 '21 06:10 devmotion