Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
devmotion opened this issue Oct 8, 2021 · 1 comment
Open

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

devmotion opened this issue Oct 8, 2021 · 1 comment

Comments

@devmotion
Copy link
Member

devmotion commented Oct 8, 2021

This issue, or rather discussion of the desired API, came up in #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
Copy link
Member Author

An additional argument for removing the type aliases and constructors is #51: It shows that (it seems) one could remove Link (and also AbstractLink) completely, as suggested by @st-- in #43.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant