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

Add typemin, typemax, ::Type version of eps. #108

Merged
merged 3 commits into from
Apr 11, 2018

Conversation

tkoolen
Copy link
Contributor

@tkoolen tkoolen commented Apr 2, 2018

A few missing methods needed to use IntervalArithmetic with RigidBodyDynamics.jl.

Just a note: I was surprised that eps(::Interval{T}) returns a T, not an Interval{T}. This isn't causing any trouble for me, but it's perhaps a little surprising that eps(::Type{Interval{T}}) now returns a T (to be consistent with eps(::Interval{T})), whereas e.g. one(::Type{Interval{T}}) returns an Interval{T}.

Thanks for the package!

@coveralls
Copy link

coveralls commented Apr 2, 2018

Coverage Status

Coverage increased (+0.04%) to 92.85% when pulling 4421fe6 on tkoolen:tk/typemin-typemax-eps into 373b19b on JuliaIntervals:master.

@codecov-io
Copy link

codecov-io commented Apr 2, 2018

Codecov Report

Merging #108 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #108      +/-   ##
==========================================
+ Coverage   92.81%   92.84%   +0.03%     
==========================================
  Files          21       21              
  Lines         960      965       +5     
==========================================
+ Hits          891      896       +5     
  Misses         69       69
Impacted Files Coverage Δ
src/IntervalArithmetic.jl 100% <ø> (ø) ⬆️
src/intervals/arithmetic.jl 97.6% <100%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 373b19b...4421fe6. Read the comment docs.

@dpsanders
Copy link
Member

Thanks very much for the contribution!

I agree about eps. Do you remember if there was a particular reason for that, @lbenet ?

@lbenet
Copy link
Member

lbenet commented Apr 3, 2018

I don't remember what was the motivation for such a definition. It is a fair point that it should behave as one.

@tkoolen Thanks for the contribution!

@tkoolen
Copy link
Contributor Author

tkoolen commented Apr 3, 2018

OK, made the eps change. I had to change dist as well, as it was (interestingly) causing the following test to fail:

a = @interval(0.1)
b = Interval(0.1, 0.1)
@test dist(a,b) <= eps(a)

This is because <=(x::Float64, y::Interval{Float64}) calls promote(x, y), which calls convert(Interval{Float64}, x), which creates a nontrivial interval, so that the test doesn't pass anymore. Does that qualify as a (minor) bug? It seems like having a convert(::Type{Interval{T}}, x::T) where {T} = Interval{T}(x) would make sense.

@tkoolen
Copy link
Contributor Author

tkoolen commented Apr 4, 2018

I tried what I proposed above, or rather

convert(::Type{Interval{T}}, x::T) where {T<:Real} = Interval{T}(x)
convert(::Type{Interval{T}}, x::T) where {T<:AbstractFloat} = Interval{T}(x)

but that resulted in a bunch of test failures.

@dpsanders
Copy link
Member

Sorry for the delay in replying.

I'm pretty sure there are convert methods already?

@dpsanders
Copy link
Member

With latest release:


julia> using IntervalArithmetic

julia> convert(Interval{Float64}, 3.1)
[3.09999, 3.10001]

or maybe I'm missing something?

@tkoolen
Copy link
Contributor Author

tkoolen commented Apr 6, 2018

Sure, there is a convert method, but I expected it to create a trivial interval, which is not the case on master. Initially, I thought the test failure was just due to the fact that the Interval created in

function convert(::Type{Interval{T}}, x::S) where {T<:AbstractFloat, S<:AbstractFloat}

is overly conservative, but now I think it's just buggy.

On master:

julia> using IntervalArithmetic

julia> a = @interval(0.1);

julia> epsa = eps(a)
1.3877787807814457e-17

julia> epsa_interval = convert(Interval{Float64}, epsa)
[1.38777e-17, 1.38778e-17]

julia> epsa  epsa_interval # !!!
false

and

julia> b = Interval(0.1, 0.1);

julia> d = dist(a,b)
1.3877787807814457e-17

julia> d_interval = convert(Interval{Float64}, d)
[1.38777e-17, 1.38778e-17]

julia> d  d_interval # again: !!!
false

julia> d < d_interval.hi
true

julia> d < d_interval.lo
true

julia> d <= eps(a) # the test on master
true

julia> d_interval <= Interval(eps(a)) # essentially the test on the PR branch if `dist` returns a Float64
false

@dpsanders
Copy link
Member

dpsanders commented Apr 9, 2018

Sorry for the delay in getting to this.

There is a problem here: using Interval to construct intervals is now (since v0.12) a bad idea, since for efficiency it no longer performs validity checks. Instead, interval (with a small i) should be used. (The documentation is quite out of date, sorry.)

julia> Interval(Inf)
[∞, ∞]

julia> interval(Inf)
ERROR: ArgumentError: `[Inf, Inf]` is not a valid interval. Need `a ≤ b` to construct `interval(a, b)`.
Stacktrace:
 [1] interval(::Float64, ::Float64) at /Users/dpsanders/.julia/v0.6/IntervalArithmetic/src/intervals/intervals.jl:100
 [2] interval(::Float64) at /Users/dpsanders/.julia/v0.6/IntervalArithmetic/src/intervals/intervals.jl:106

Intervals with two Infs are not allowed (by the IEEE standard for interval arithmetic).

I propose using something like

julia> typemax(x::Interval{T}) where T = wideinterval(convert(T, Inf))

julia> IntervalArithmetic.wideinterval(Inf)
[1.79769e+308, ∞]

@tkoolen tkoolen force-pushed the tk/typemin-typemax-eps branch from 9931f46 to 4421fe6 Compare April 11, 2018 18:19
@tkoolen
Copy link
Contributor Author

tkoolen commented Apr 11, 2018

OK, thanks. Rebased onto master and addressed your comment. Since wideinterval is only defined for <:AbstractFloat, I decided to special-case <:Integer. I wasn't sure what to do with <:Rational, so I just didn't define methods for <:Rational.

@tkoolen
Copy link
Contributor Author

tkoolen commented Apr 11, 2018

By the way, the conservative widening part of #108 (comment) was also addressed by #114, so I was able to undo my changes to dist (having it return a number instead of an Interval, as before).


typemin(::Type{Interval{T}}) where T<:AbstractFloat = wideinterval(typemin(T))
typemax(::Type{Interval{T}}) where T<:AbstractFloat = wideinterval(typemax(T))
typemin(::Type{Interval{T}}) where T<:Integer = Interval(typemin(T))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The package is not supposed to be used with integer intervals, but I guess no harm is done!

@dpsanders dpsanders merged commit 801b6c5 into JuliaIntervals:master Apr 11, 2018
@dpsanders
Copy link
Member

Many thanks @tkoolen!

Do you have an example of using the package with RigidBodyDynamics.jl?

@tkoolen tkoolen deleted the tk/typemin-typemax-eps branch April 11, 2018 22:55
@tkoolen
Copy link
Contributor Author

tkoolen commented Apr 11, 2018

You're very welcome.

Do you have an example of using the package with RigidBodyDynamics.jl?

Yeah, a simple demo over at https://github.com/JuliaRobotics/RigidBodyDynamics.jl/blob/tk/interval-arithmetic/notebooks/Rigorous%20error%20bounds%20using%20IntervalArithmetic.ipynb (currently on a branch). I'm putting together some additional notebooks, partly for two upcoming tutorials. Could I bother you for a new tag?

@dpsanders
Copy link
Member

Very nice! I have a few modifications to suggest for that notebook -- shall I make a PR or just list them here? (And is there now a decent story about diffs for notebooks?

@tkoolen
Copy link
Contributor Author

tkoolen commented Apr 12, 2018

You can just list them here, that's probably easier. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants