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

RFC: making parameters subtypes of their originals

Open simsurace opened this issue 1 year ago • 8 comments
trafficstars

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.

simsurace avatar Feb 27 '24 17:02 simsurace

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.

willtebbutt avatar Feb 28 '24 15:02 willtebbutt

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.

simsurace avatar Feb 28 '24 18:02 simsurace

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.

willtebbutt avatar Feb 29 '24 11:02 willtebbutt

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

simsurace avatar Feb 29 '24 12:02 simsurace

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)

willtebbutt avatar Feb 29 '24 12:02 willtebbutt

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.

simsurace avatar Feb 29 '24 12:02 simsurace

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

willtebbutt avatar Feb 29 '24 12:02 willtebbutt

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

simsurace avatar Feb 29 '24 13:02 simsurace