JuliaGaussianProcesses / ParameterHandling.jl

Foundational tooling for handling collections of parameters in models
MIT License
72 stars 11 forks source link

RFC: making parameters subtypes of their originals #74

Open simsurace opened 8 months ago

simsurace commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

willtebbutt commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

simsurace commented 8 months ago

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