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

Make ARDTransform a subtype of LinearTransform? #107

Open
devmotion opened this issue Apr 27, 2020 · 4 comments
Open

Make ARDTransform a subtype of LinearTransform? #107

devmotion opened this issue Apr 27, 2020 · 4 comments

Comments

@devmotion
Copy link
Member

As discussed in #106, the ARDTransform could be viewed as a special LinearTransform with a diagonal matrix. Do we still want to keep these separate types?

@theogf
Copy link
Member

theogf commented Apr 27, 2020

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

@willtebbutt
Copy link
Member

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.

@devmotion
Copy link
Member Author

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.

@theogf
Copy link
Member

theogf commented Apr 28, 2020

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

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

3 participants