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

Remove IntervalBox in favor of AbstractVector{Interval} #550

Closed
Kolaru opened this issue Nov 29, 2022 · 11 comments · Fixed by #582
Closed

Remove IntervalBox in favor of AbstractVector{Interval} #550

Kolaru opened this issue Nov 29, 2022 · 11 comments · Fixed by #582
Assignees
Labels
1.0 Planned for the major 1.0 release enhancement

Comments

@Kolaru
Copy link
Collaborator

Kolaru commented Nov 29, 2022

We discuss we Luca and it looks like we could get rid of IntervalBox alltogether, and just define its methods on AbstractVector{Interval}.

It would unify things, as a mutlidimensional function extended to intervals would always return a Vector{Interval}. This is currently a slight annoyance in IntervalRootFinding.

It doesn't feel like it is type piracy (no clash is possible with other modules), and it doesn't seem to cause any problem (we only have few methods on IntervalBox and many are to emulate a vector).

@lucaferranti @dpsanders @lbenet

@Kolaru Kolaru added 1.0 Planned for the major 1.0 release need discussion labels Nov 29, 2022
@dpsanders
Copy link
Member

Hmm that actually doesn't seem like the right approach to me, since an interval box is conceptually different from just "a vector of intervals".

It is possible that you would want to somehow parametrize IntervalBox on the type of vector it contains, e.g. Vector or SVector.

@OlivierHnt
Copy link
Member

I have been wondering about this myself, what is conceptually different between IntervalBox and AbstractVector{<:Interval}? What operations are specific to IntervalBox and would not make sense for a general vector of Interval?

@dpsanders
Copy link
Member

e.g. diameter (i.e. the max width in some dimension), volume.

@OlivierHnt
Copy link
Member

From an outsider perspective, for any instance of AbstractVector{<:Interval}, both diameter and volume seem to be also well-defined notions. What am I overlooking?

@lucaferranti
Copy link
Member

from a type piracy perspective, I think only set functions could be problematic

julia> setdiff([1..2, 3..4], [0..1, 1..2])
1-element Vector{Interval{Float64}}:
 [3, 4]

julia> setdiff(IntervalBox([1..2, 3..4]), IntervalBox([0..1, 1..2]))
1-element Vector{IntervalBox{2, Float64}}:
 [1, 2] × [3, 4]

julia> [1..2, 3..4]  [0..3, 1..5]
false

julia> IntervalBox([1..2, 3..4])  IntervalBox([0..3, 1..5])
true

and even that case above is maybe borderline whether is type piracy or not.

I don't think diam is a problem, because it's not defined in Base or any package IA depends on, so it's not type piracy

@OlivierHnt
Copy link
Member

Ah indeed, set functions are ambiguous. The type IntervalBox{N} enforces the (N-dimensional equivalent) set behaviour of Interval.

@lbenet
Copy link
Member

lbenet commented Dec 2, 2022

I'm not sure if this proposal incurs in type-piracy or not. Yet, it seems to me that the right approach is to define a struct for what we currently call an IntervalBox, as we do it. If we want it to behave much more like and AbstractVector perhaps we should impose that it is a (parametric) subtype of an AbstractVector.

Would that help unifying things?

@Kolaru
Copy link
Collaborator Author

Kolaru commented Dec 4, 2022

Having our own struct is however limited, because the interval extension of a user defined function will never return an IntervalBox. Most of the time, a function f(x)::Vector{Number} becomes f(x::Interval}::Vector{Interval}.

It looks to me like the use of IntervalBox is limited to distinguishing few corner case (e.g. setdiff). It does indeed feel natural (the other solution I can see would be to define a new function eg intervalbox_setdiff), and I am fine with keeping it as is.

We could have definition for Vector{Interval} in addition to IntervalBoxes for the common cases that are unambiguous.

@aafsar
Copy link

aafsar commented May 10, 2023

Please excuse me if there is a very apparent answer to this, but I'm curious why Interval{T} and IntervalBox{N, T} are not subtypes of a commom supertype, e.g. AbstractInterval{T} (or that IntervalBox is not a subtype of AbstractInterval, which already exists)? Isn't it the case that conceptually IntervalBox is a generalization of Interval for multidimensional spaces? Of course there can be other generalizations (e.g. IntervalBall), but all those generalizations can be thought under the concept of AbstractInterval.

From multiple dispatch point of view, the upstream packages in the ecosystem, and applications using the ecosystem, might benefit from a common supertype. Just to give a very simple example from 'IntervalOptimisation':

numeric_type(::Interval{T}) where {T} = T
numeric_type(::IntervalBox{N, T}) where {N, T} = T

@OlivierHnt
Copy link
Member

I think one difficulty in having Interval and IntervalBox have a common supertype is that we rely on having Interval{T} <: Real.

@Kolaru
Copy link
Collaborator Author

Kolaru commented Jul 24, 2023

Discussed in triage call, and the consensus is that it is reasonable to remove it.

@OlivierHnt OlivierHnt self-assigned this Jul 24, 2023
@OlivierHnt OlivierHnt linked a pull request Sep 5, 2023 that will close this issue
@OlivierHnt OlivierHnt removed a link to a pull request Sep 5, 2023
@OlivierHnt OlivierHnt linked a pull request Sep 5, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 Planned for the major 1.0 release enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants