-
Notifications
You must be signed in to change notification settings - Fork 71
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
add IntervalSets conversions #677
Conversation
Thanks for opening this PR. Looks reasonable to have this extension 🙂. |
Mhmm I am not sure we want to have a conversion between PS: as mentioned before I am not opposed to this PR, it seems very reasonable for interval libraries to communicate. Though I am curious 🙂, is there any specific reason that motivated opening the PR? |
IS.Interval(i::IA.BareInterval) = IS.Interval(IA.inf(i), IA.sup(i)) | ||
|
||
# Interval <- IS.Interval | ||
Base.convert(::Type{IA.Interval}, i::IS.Interval{<:Any,<:Any,T}) where {T} = convert(IA.Interval{float(T)}, i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this line, it seems that IS.Interval
can have bounds that are of different real types (eg Rational
, or even maybe Int
)?
In our case, IA.Interval
can have bounds that are either Rational
or AbstractFloat
.
Hence, if you would like to preserve as much as possible the same bound type as the initial interval, I suggest the following:
Base.convert(::Type{IA.Interval}, I::IS.Interval{<:Any,<:Any,T}) where {T} = convert(IA.Interval{IA.promote_numtype(T, T)}, i)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally, makes sense! Didn't know this promote function.
To represent intervals "in general", not specifically as a kind of numbers, IntervalSets are a great choice. They are lightweight, popular, and well-supported by other libraries, they define just the right amount of functionality. But sometimes, for stuff like arithmetics or bounding function values, IntervalArithmetics are needed. Then one has all these questions on how to convert back and forth: xint = 1..5 # IntervalSets interval, comes from external code
yint = Interval(myfunc(IA.interval(xint))) # bound myfunc() values on xint
# yint can be used with anything supporting IntervalSets For now, I have to copy-paste such conversion definitions whenever such a procedure is needed. Would be nice to have them packaged here! |
Not at all, they are "intervals as sets", not "intervals as numbers". |
Yes it affects the So if an Then you could implement something like # BareInterval <- IS.Interval
IA.bareinterval(i::IS.Interval) = IA.bareinterval(IS.endpoints(i)...)
IA.bareinterval(::Type{T}, i::IS.Interval) where {T<:IA.NumTypes} = IA.bareinterval(T, IS.endpoints(i)...)
Base.convert(::Type{IA.BareInterval}, i::IS.Interval) = IA.bareinterval(i)
Base.convert(::Type{IA.BareInterval{T}}, i::IS.Interval) where {T<:IA.NumTypes} = IA.bareinterval(T, i)
# BareInterval -> IS.Interval
IS.Interval(i::IA.BareInterval) = IS.Interval(IA.inf(i), IA.sup(i))
# maybe also IS.Interval{T,S,R}(i::IA.BareInterval) where {T,S,R} = IS.Interval{T,S,R}(IA.inf(i), IA.sup(i))
Base.convert(::Type{IS.Interval}, i::IA.BareInterval) = IS.Interval(i)
# maybe you want to add a conversion to a more specific signature, eg Base.convert(::Type{T}, i::IA.BareInterval) where {T<:IS.BareInterval} = T(i)
# Interval <- IS.Interval
IA.interval(i::IS.Interval) = IA.interval(IS.endpoints(i)...)
IA.interval(::Type{T}, i::IS.Interval) where {T<:IA.NumTypes} = IA.interval(T, IS.endpoints(i)...)
Base.convert(::Type{IA.Interval}, i::IS.Interval) = IA.interval(i)
Base.convert(::Type{IA.Interval{T}}, i::IS.Interval) where {T<:IA.NumTypes} = IA.interval(T, i)
# Interval -> IS.Interval
IS.Interval(i::IA.Interval) = IS.Interval(IA.inf(i), IA.sup(i))
# maybe also IS.Interval{T,S,R}(i::IA.Interval) where {T,S,R} = IS.Interval{T,S,R}(IA.inf(i), IA.sup(i))
Base.convert(::Type{IS.Interval}, i::IA.Interval) = IS.Interval(i)
# maybe you want to add a conversion to a more specific signature, eg Base.convert(::Type{T}, i::IA.Interval) where {T<:IS.Interval} = T(i) |
A more perverse potential issue: does |
No, IntervalSets is quite strict and defines basically no implicit conversions. The only |
Ok I looked at IntervalSets.jl. I found two issues (from the point of view of IntervalArithmetic.jl): julia> using IntervalSets
julia> Interval(1e-120, 1e-119) # non trivial interval set
1.0e-120 .. 1.0e-119
julia> ClosedInterval{Float32}(1e-120 .. 1e-119) # issue #1: conversion did not widen the interval set
0.0 .. 0.0
julia> 1 ± 1e-120 # issue #2: the interval only contains 1
1.0 .. 1.0 If these are intentional, then we should not convert to # Interval <- IS.Interval
function IA.interval(i::IS.Interval)
x = IA.interval(IS.endpoints(i)...)
return IA._unsafe_interval(IA.bareinterval(x), IA.decoration(x), false)
end
function IA.interval(::Type{T}, i::IS.Interval) where {T<:IA.NumTypes}
x = IA.interval(T, IS.endpoints(i)...)
return IA._unsafe_interval(IA.bareinterval(x), IA.decoration(x), false)
end
Base.convert(::Type{IA.Interval}, i::IS.Interval) = IA.interval(i)
Base.convert(::Type{IA.Interval{T}}, i::IS.Interval) where {T<:IA.NumTypes} = IA.interval(T, i)
# Interval -> IS.Interval
IS.Interval(i::IA.Interval) = IS.Interval(IA.inf(i), IA.sup(i))
# maybe also IS.Interval{T,S,R}(i::IA.Interval) where {T,S,R} = IS.Interval{T,S,R}(IA.inf(i), IA.sup(i))
Base.convert(::Type{IS.Interval}, i::IA.Interval) = IS.Interval(i)
# maybe you want to add a conversion to a more specific signature, eg Base.convert(::Type{T}, i::IA.Interval) where {T<:IS.Interval} = T(i) |
Thanks for looking into it! Updated the code. |
It looks very reasonable to me. There is just one edge case: IA intervals are always closed, except when one of the bounds is infinity, then the interval is always open on the side of the infinity. So accordingly, |
Indeed. Also this raises the question for NaI and empty intervals; @aplavin do you have a special signature for "empty intervals"? |
Hmm, I guess makes sense, even though it would be type-unstable...
Not really, IS.Interval is fully defined by its endpoints – there are no other flags. |
Yes sadly. So I am a bit puzzled, with IntervalSets you can construct |
No, it's an empty interval – no special casing, just following the regular interval definition: help?> Interval
search: Interval IntervalSets OpenInterval ClosedInterval AbstractInterval searchsorted_interval InteractiveUtils isinteractive
An Interval{L,R}(left, right) where L,R are :open or :closed is an interval set containg x such that
1. left ≤ x ≤ right if L == R == :closed
2. left < x ≤ right if L == :open and R == :closed
3. left ≤ x < right if L == :closed and R == :open, or
4. left < x < right if L == R == :open Naturally, there's no julia> 1.5 ∈ Interval(1, 2)
true
julia> 1.5 ∈ Interval(2, 1)
false
julia> isempty(Interval(1, 2))
false
julia> isempty(Interval(2, 1))
true |
Oh ok, for us all empty intervals have the convention that Mhmm maybe there are multiple ways of handling this; e.g. IA.interval(i::IS.Interval{L,R,T}) where {L,R,T} = IA.interval(IA.promote_numtype(T, T), i)
function IA.interval(::Type{T}, i::IS.Interval{L,R}) where {T<:IA.NumTypes,L,R}
lo, hi = IS.endpoints(i)
isinf(lo) & (L == :closed) && return IA.nai(T) # eg IS.Interval(-Inf, 1) is invalid since closed
isinf(hi) & (R == :closed) && return IA.nai(T) # eg IS.Interval(1, Inf) is invalid since closed
L == :open && return IA.nai(T) # eg IS.Interval{:open,:closed}(1, 2) is invalid since not closed
R == :open && return IA.nai(T) # eg IS.Interval{:closed,:open}(1, 2) is invalid since not closed
x = IA.interval(T, lo, hi)
return IA._unsafe_interval(IA.bareinterval(x), IA.decoration(x), false)
end
function IS.Interval(i::IA.Interval)
lo, hi = IA.bounds(i)
L = ifelse(isinf(lo), :open, :closed)
R = ifelse(isinf(hi), :open, :closed)
return IS.Interval{L,R}(lo, hi)
end which yield the following behaviour (I let you decide if it is appropriate for IntervalSets.jl) julia> x = IA.interval(1, 2); y = IA.interval(1, Inf); z = IA.interval(-Inf, Inf); # valid intervals according to IA
julia> IS.Interval(x), IS.Interval(y), IS.Interval(z)
(1.0 .. 2.0, 1.0 .. Inf (closed-open), -Inf .. Inf (open))
julia> u = IA.interval(2, 1); # invalid interval according to IA
┌ Warning: ill-formed interval [a, b] with a = 2, b = 1 and decoration d = com. NaI is returned
└ @ IntervalArithmetic ~/.julia/dev/IntervalArithmetic/src/intervals/construction.jl:444
julia> IS.Interval(u)
NaN .. NaN
julia> v = IA.emptyinterval(); # empty interval according to IA
julia> IS.Interval(v)
Inf .. -Inf (open) |
These would be quite unfortunate, given that Other than that, I fully support your suggested implementation! |
Mhmm yeah. What differs when using |
I think you are right, and the safest approach is to disallow converting |
Updated the PR implementation and tests. Tagging @hyrodium in case he has some comments on the conversion. |
Should be ready I guess :) @OlivierHnt |
Sorry I was traveling. Merging now 🙂 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #677 +/- ##
==========================================
+ Coverage 81.02% 81.20% +0.18%
==========================================
Files 27 28 +1
Lines 2324 2336 +12
==========================================
+ Hits 1883 1897 +14
+ Misses 441 439 -2 ☔ View full report in Codecov by Sentry. |
* add IntervalSets conversions * upd Project * fixes * upd * more careful conversion
No description provided.