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

Make default bounds type Interval from IntervalSets

Open bgroenks96 opened this issue 3 years ago • 9 comments

This is a minor issue, but I think it would make more sense to use IntervalSets.Interval for the bounds field of Param (this goes for FieldMetadata as well). As far as I can tell, there is nothing that stops you from doing this at the moment, but since the type of bounds is listed as Tuple in FieldMetadata, I assume there is some code somewhere that assumes it will be a tuple of numbers..?

Anyway, IntervalSets provides nice syntax for closed intervals 0..1, membership checks with , automatically handles casting between number types (e.g. declaring the interval with Int but using it with Float64 values), and of course properly distinguishes between open and closed intervals.

I don't really see any reason not to use it as the standard type for bounds.

bgroenks96 avatar Sep 20 '21 11:09 bgroenks96

I've just been considering swapping DimensionalData.jl over to that too. I wasn't really aware of the package previously.

But the problem with defaults provided by packages is we have to depend on the package. And we want to keep this as small as possible.

There is code in InteractModels.jl that assumes that assumes bounds is a Tuple:

https://github.com/rafaqz/ModelParameters.jl/blob/master/InteractModels/src/interactive.jl#L241-L263

Can we generalise that to Intervals without needing the dependency? E.g. just using first and last ? The main requirement in that case is something we can make sliders from. so a tuple should continue to work... a length 2 vector should probably also work.

rafaqz avatar Sep 20 '21 12:09 rafaqz

IntervalSets is a pretty lightweight package, I think (only three dependencies and a couple hundred lines of code). So I probably wouldn't worry about depending on it. For most users it will probably already be in the package environment.

But I guess we could generalize it. So one can use any type for bounds but it must simply provide first and last as the bounds? Is that the idea?

bgroenks96 avatar Sep 20 '21 12:09 bgroenks96

I always try not to add a dependency... They add up in compile time and CI run time, and it adds maintenance burden.

Also thinking about this, doesn't Optim.jl take tuple bounds too? Or maybe two vectors. But I don't know if it handles intervals. That's where it comes from originally and why it's the default. I think using Tuple/two fixed values is the more common use case than Interval.

Interval also doesn't seem to define first and last. So therye hard to handle genericaly. I guess the open/closed thing complicates that anyway.

I'm not sure how we are meant to handle that for Optim.jl or making sliders from intervals either. You would have to use eps etc. and make sure it works with all types. I would prefer not to worry about that without a really pressing use-case.

rafaqz avatar Sep 20 '21 13:09 rafaqz

Also thinking about this, doesn't Optim.jl take tuple bounds too? Or maybe two vectors. But I don't know if it handles intervals.

It takes two vectors. So it doesn't really work "out of the box" with the tuples either. You would need to map over the Params with first and last to produce the upper and lower bounds vectors.

Interval also doesn't seem to define first and last. So therye hard to handle genericaly. I guess the open/closed thing complicates that anyway.

Yeah... I suppose that's intentional. But this is a legitimate problem that should be considered in many cases. Several of the parameters I deal with have open lower bounds, for example (zero to infinity, open on both ends). This is, in my opinion, an advantage of Interval because it forces you to be explicit about this, whereas the Tuple is kind of ambiguous. I suppose the general assumption is that it's closed..? I think that's the assumption from Optim.

Yes, you would have to use eps() for the open case... but shouldn't you kind of be doing that anyway? If you have a parameter with an open boundary, you by definition should not allow the slider or optimizer to use the exact value of the bound. That seems like a pressing use-case to me!

bgroenks96 avatar Sep 20 '21 14:09 bgroenks96

I think you're right about sliders and sets, not including zero is often very useful.

But using intervals is definitely more complicated than Tuple... If you want to take that on and make PR I can go with that. But we should make sure that:

  • common types work as bounds, remember Int doesn't have eps but are valid bounds values.
  • Tuple bounds still work for sliders
  • there is an easy way to get Optim.jl bounds from intervals.

rafaqz avatar Sep 20 '21 15:09 rafaqz

Where is the code currently that actually handles the bounds field?

bgroenks96 avatar Sep 20 '21 15:09 bgroenks96

https://github.com/rafaqz/ModelParameters.jl/blob/master/InteractModels/src/interactive.jl#L196-L202

Not so elegent, but it works. The idea is that a range is preferable for a slider, but we can work with bounds too, which are more often used for other things. Bounds for Optim etc are probably the same as the bounds you wan to investigate manually.

withunits hacks in units from a separate units field, as sliders can have them while the optimiser probably can't. And it's often nice to keep them separate for other reasons too - the bounds and the main value have the same units and specifying that three times is annoying.

rafaqz avatar Sep 20 '21 15:09 rafaqz

I really should comment that code a bit more

rafaqz avatar Sep 20 '21 15:09 rafaqz

Ok, I'll take a look. It doesn't seem like it should be that hard...

Regarding the different types issue, I think the open/closed distinction only makes sense for real numbers. An open integer set doesn't really make sense because you could just equivalently define a closed set with the next integer. So we don't have to provide method dispatches for open integer sets, I think.

bgroenks96 avatar Sep 20 '21 15:09 bgroenks96