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

RFC: making parameters subtypes of their originals #74

Open
simsurace opened this issue Feb 27, 2024 · 8 comments
Open

RFC: making parameters subtypes of their originals #74

simsurace opened this issue Feb 27, 2024 · 8 comments

Comments

@simsurace
Copy link
Member

The AbstractParameters supertype does not seem to be used and there has not been a need for it in my experience.
What if we made the parameters subtypes of their originals, e.g. what if positive(1.0) isa Real or positive_definite(A) isa AbstractMatrix{eltype{A}}? Overloading methods to arithmetic would make it so we can stick those parameters almost everywhere and be unpacked automatically.

@willtebbutt
Copy link
Member

I agree with getting rid of the supertyping -- it feels redundant in that I think we really just want a trait-based system.

However, I'm not of the opinion that we should start making our types subtype things like Real and AbstractMatrix{<:Real} or whatever, as I really like having the clean separation of the "parameter" representation, and the "exactly the types you expect with no parameter stuff" representation. The advantage of this approach is that you know for sure that you can write your model code without think about ParameterHandling.jl stuff, and use value_unflatten to be confident that you've got things of exactly the correct type.

@simsurace
Copy link
Member Author

Hmm I'm not sure I see the downside yet. Wouldn't it just be nice if we could just do positive(2.0) * SEKernel() and nobody would bat an eye?
The idea being that something like EasyGPs could reuse those constraints if present.
It of course does not need to be implemented in this package, it could work as another package.

@willtebbutt
Copy link
Member

Hmm I'm not sure I see the downside yet. Wouldn't it just be nice if we could just do positive(2.0) * SEKernel() and nobody would bat an eye?

This is a good point. I'd definitely be interested to see what this would look like in practice.

I gues in situations where you don't need parameter tying, this approach works well. It's just that have to be a little bit careful with parameter tying with this kind of approach. e.g. if you want something like

kernel(l) = with_lengthscale(SEKernel(), l) + with_lengthscale(Matern12Kernel(), l)

it's not clear to me that you can get away with writing it as

l = positive(1.0)
k= with_lengthscale(SEKernel(), l) + with_lengthscale(Matern12Kernel(), l)

and then trying to extract the fact that you want to tie the lengthscales from k.

@simsurace
Copy link
Member Author

No, I don't think the latter will work like that. But you could apply the lengthscale to the sum, right?

@willtebbutt
Copy link
Member

Good point. There are definitely presumably more complicated parameter tying settings that you might consider though. e.g. I don't know why you would want this, but something like

kernel(a) = a * with_lengthscale(SEKernel(), a)

@simsurace
Copy link
Member Author

simsurace commented Feb 29, 2024

I think I know how to address this with something like a ParameterizedKernel(a -> * with_lengthscale(SEKernel(), a), positive(1.0)). But I do see the point here.
For now, I'll explore the idea with a few additional wrappers and see where I end up with.

@willtebbutt
Copy link
Member

Oooo I like this ParametrizedKernel idea -- feels like it would be very straightforward to implement, and potentially really quite clear to the user.

@simsurace
Copy link
Member Author

Lots of low-hanging fruit still hanging around I think :)

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

2 participants