Introduce `atomic` function, extracted out of `convert` #114

tkoolen · 2018-04-06T05:55:33Z

closure essentially does what convert did previously.
convert will now avoid calling closure when doing so makes sense
all explicit uses of convert in the package are replaced with closure

Result:

julia> x = 0.1..0.2
[0.0999999, 0.200001]

julia> @btime 3.1 + $x
  19.124 ns (0 allocations: 0 bytes)
[3.19999, 3.30001]

Fixes #104.

I'm not married to the name closure, but it kind of works.

~~Tests almost pass, but there's a couple of failures for tan; this is the first:~~

IntervalArithmetic.jl/test/ITF1788_tests/libieeep1788_tests_elem.jl

Line 3629 in e6375b0

    
           @test tan(Interval(-0x1.921fb54442d18p+0, 0x1.921fb54442d18p+0)) == Interval(-0x1.d02967c31cdb5p+53, 0x1.d02967c31cdb5p+53)

~~lo_quadrant appears to be wrong. Any idea why that would be the case?~~

Never mind, already fixed it; all tests pass now.

codecov-io · 2018-04-06T06:46:15Z

Codecov Report

Merging #114 into master will increase coverage by 0.11%.
The diff coverage is 84.84%.

@@            Coverage Diff             @@
##           master     #114      +/-   ##
==========================================
+ Coverage   92.69%   92.81%   +0.11%     
==========================================
  Files          21       21              
  Lines         958      960       +2     
==========================================
+ Hits          888      891       +3     
+ Misses         70       69       -1

Impacted Files	Coverage Δ
src/intervals/precision.jl	`91.66% <100%> (ø)`	⬆️
src/decorations/intervals.jl	`91.42% <100%> (ø)`	⬆️
src/intervals/hyperbolic.jl	`100% <100%> (ø)`	⬆️
src/intervals/macros.jl	`100% <100%> (ø)`	⬆️
src/intervals/intervals.jl	`85.71% <100%> (+0.42%)`	⬆️
src/intervals/trigonometric.jl	`100% <100%> (ø)`	⬆️
src/intervals/conversion.jl	`81.81% <71.42%> (+5.62%)`	⬆️
src/intervals/functions.jl	`97.11% <83.33%> (ø)`	⬆️

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 e6375b0...709ece7. Read the comment docs.

coveralls · 2018-04-06T06:46:43Z

Coverage decreased (-0.08%) to 92.612% when pulling 2f56703 on tkoolen:tk/closure into e6375b0 on JuliaIntervals:master.

coveralls · 2018-04-06T06:46:43Z

Coverage increased (+0.1%) to 92.813% when pulling 709ece7 on tkoolen:tk/closure into e6375b0 on JuliaIntervals:master.

lbenet · 2018-04-06T16:05:03Z

src/intervals/conversion.jl

    isinf(x) && return wideinterval(T(x))

    Interval{T}( T(x, RoundDown), T(x, RoundUp) )
    # the rounding up could be done as nextfloat of the rounded down one?
    # use @round_up and @round_down here?
 end

-function convert(::Type{Interval{T}}, x::S) where {T<:AbstractFloat, S<:AbstractFloat}
+function closure(::Type{Interval{T}}, x::S) where {T<:AbstractFloat, S<:AbstractFloat}
    isinf(x) && return wideinterval(x)#Interval{T}(prevfloat(T(x)), nextfloat(T(x)))


A small remark, which is not due to the changes you propose (perhaps it is me the one to be blamed!), but this is a good chance to correct it: This method is not type stable. To be type stable, I think it should return wideinterval(T(x)).

An example that shows the problem is:

julia> typeof( IntervalArithmetic.closure(Interval{BigFloat}, Inf) ) IntervalArithmetic.Interval{Float64} julia> typeof( IntervalArithmetic.closure(Interval{BigFloat}, 0.1) ) IntervalArithmetic.Interval{BigFloat}

While I agree this change should be made, I think it should be made in a separate pull request.

As you prefer...

lbenet

@tkoolen Thanks a lot for the contribution! I have taken a look on this PR and I am totally in favor of merging it. The only comment I have is related to a type-unstable method, which comes from the past, but it is a good occasion to correct it.

I agree with your comments (in the tests) about the undesirability of certain behavior, e.g. that
convert(Interval, 0.1) == Interval(0.09999999999999999, 0.1) returns true. This is in a sense consistent with the behavior of Interval(0.1,0.1), which is not properly rounded (nor checked that the lower bound is less-or-equal to the upper one). Since this may appear to be "not so natural", I think it is worth to write it somewhere (docs, and perhaps in the docstrings of closure)! And to emphasize this point, it is worth to have the proper tests.

tkoolen

Alright, so still to do:

documentation (docstring for closure)
remove previous tests with undesired behavior instead of just commenting them out
decide whether the convert methods that aren't getting called in tests should be deleted or should be covered by additional tests
test coverage for ^(a::Interval{Rational{T}}, x::AbstractFloat) where T<:Integer and ^(a::Interval{Rational{T}}, x::AbstractFloat) where T<:Integer
test coverage for closure(::Type{Interval{Rational{Int}}}, x::Irrational)
and closure(::Type{Interval{Rational{BigInt}}}, x::Irrational)

tkoolen · 2018-04-06T17:11:31Z

src/intervals/conversion.jl

    isinf(x) && return wideinterval(T(x))

    Interval{T}( T(x, RoundDown), T(x, RoundUp) )
    # the rounding up could be done as nextfloat of the rounded down one?
    # use @round_up and @round_down here?
 end

-function convert(::Type{Interval{T}}, x::S) where {T<:AbstractFloat, S<:AbstractFloat}
+function closure(::Type{Interval{T}}, x::S) where {T<:AbstractFloat, S<:AbstractFloat}
    isinf(x) && return wideinterval(x)#Interval{T}(prevfloat(T(x)), nextfloat(T(x)))


While I agree this change should be made, I think it should be made in a separate pull request.

tkoolen · 2018-04-06T18:13:00Z

Re: test coverage for closure(::Type{Interval{Rational{Int}}}, x::Irrational)
and closure(::Type{Interval{Rational{BigInt}}}, x::Irrational): these weren't covered on master either and are in fact broken:

julia> convert(Interval{Rational{Int}}, π)
ERROR: MethodError: Cannot `convert` an object of type IntervalArithmetic.Interval{BigFloat} to an object of type IntervalArithmetic.Interval{Rational{Int64}}
This may have arisen from a call to the constructor IntervalArithmetic.Interval{Rational{Int64}}(...),
since type constructors fall back to convert methods.
Stacktrace:
 [1] convert(::Type{IntervalArithmetic.Interval{Rational{Int64}}}, ::Irrational{:π}) at /home/twan/code/RigidBodyDynamics/v0.6/IntervalArithmetic/src/intervals/conversion.jl:62

How should this be fixed?

Edit: similarly, on master:

julia> convert(Interval{Rational{Int}}, Interval(0.1))
ERROR: MethodError: Cannot `convert` an object of type IntervalArithmetic.Interval{Float64} to an object of type IntervalArithmetic.Interval{Rational{Int64}}
This may have arisen from a call to the constructor IntervalArithmetic.Interval{Rational{Int64}}(...),
since type constructors fall back to convert methods.

which causes

julia> Interval(5//2)^3.0
ERROR: MethodError: Cannot `convert` an object of type IntervalArithmetic.Interval{Float64} to an object of type IntervalArithmetic.Interval{Rational{Int64}}
This may have arisen from a call to the constructor IntervalArithmetic.Interval{Rational{Int64}}(...),
since type constructors fall back to convert methods.
Stacktrace:
 [1] ^(::IntervalArithmetic.Interval{Rational{Int64}}, ::Float64) at /home/twan/code/RigidBodyDynamics/v0.6/IntervalArithmetic/src/intervals/functions.jl:138

lbenet · 2018-04-06T20:37:49Z

The following is an idea, so feel free to change it as you want.

Irrationals would yield a very narrow interval, but not a tight one. It may happen that rationalizing the bounds yields one distinct rational (instead of two), which cannot be used to build an interval that contains the irrational. So then, the idea is to shift to the left and right the rationalized value, the minimum possible amount in each case, and then construct the interval.

The following function does that in a very non-optimized way:

function clausure(::Type{Interval{Rational{T}}}, x::S) where {T<:Integer, S<:Irrational}
       x_rat = rationalize(T, x)
       p_rat = numerator(x_rat)
       q_rat = denominator(x_rat)
       lo = max( p_rat//(q_rat+1), (p_rat-1)//q_rat )
       hi = min( p_rat//(q_rat-1), (p_rat+1)//q_rat )
       return Interval(lo,hi)
end

And it seems to work:

julia> π ∈ clausure(Interval{Rational{Int}}, π)
true

julia> big(π) ∈ clausure(Interval{Rational{Int}}, π)
true

julia> eu ∈ clausure(Interval{Rational{Int}}, eu)
true

julia> big(eu) ∈ clausure(Interval{Rational{Int}}, eu)
true

julia> float( diam(clausure(Interval{Rational{Int}}, π)) )
2.373975753932138e-18

julia> float( diam(clausure(Interval{Rational{Int}}, eu)) )
9.472290677529826e-19

julia> diam(@interval(π))
4.440892098500626e-16

I hope this is useful!

dpsanders · 2018-04-09T17:12:50Z

Thanks very much for the nice contribution, @tkoolen!

I've now had a chance to discuss this with @lbenet and we are very happy to merge this as is, or would you like to add the methods to fix the bugs you found? (Or should we just add it to your PR?)

tkoolen · 2018-04-09T21:44:10Z

Yeah, might be a good idea to merge now if you're sufficiently happy with it. It'd be another couple of days (maybe even a week) before I would have time to make and test the changes proposed by @lbenet.

dpsanders · 2018-04-09T22:53:54Z

I think we will definitely merge then.

My only doubt would be about the name closure. In true julia style, we should bikeshed it...

I suggest enclosure (standard in the interval community) or atomic (as in atomic - indivisible).

lbenet · 2018-04-09T22:55:22Z

I think I prefer atomic...

tkoolen · 2018-04-09T23:12:53Z

atomic it is for now (further bikeshedding can be done in a later PR).

dpsanders · 2018-04-10T01:44:36Z

Thanks again for this contribution, @tkoolen!

tkoolen added 2 commits April 6, 2018 01:36

Introduce closure function, separate from convert.

80fecff

Fix find_quadrants (implicit convert -> explicit closure).

2f56703

lbenet mentioned this pull request Apr 6, 2018

Horrible performance for operations combining floats and intervals #104

Closed

lbenet reviewed Apr 6, 2018

View reviewed changes

tkoolen commented Apr 6, 2018

View reviewed changes

tkoolen added 3 commits April 6, 2018 14:24

Add docstring to closure.

71762e1

Remove tests with undesired behavior.

5f2cd46

Remove one convert method, test another.

8f822dc

closure -> atomic

709ece7

dpsanders merged commit 373b19b into JuliaIntervals:master Apr 10, 2018

tkoolen deleted the tk/closure branch April 10, 2018 03:15

tkoolen mentioned this pull request Apr 11, 2018

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

Merged

dpsanders mentioned this pull request Apr 11, 2018

Unexpected allocations #70

Closed

dpsanders changed the title ~~Introduce closure function, extracted out of convert~~ Introduce atomic function, extracted out of convert Apr 12, 2018

dpsanders mentioned this pull request Apr 12, 2018

Add NEWS for v0.13 #122

Merged

lbenet mentioned this pull request Jun 4, 2018

Add methods to use .. notation with Irrationals #144

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `atomic` function, extracted out of `convert` #114

Introduce `atomic` function, extracted out of `convert` #114

tkoolen commented Apr 6, 2018 •

edited

Loading

codecov-io commented Apr 6, 2018 •

edited

Loading

coveralls commented Apr 6, 2018

coveralls commented Apr 6, 2018 •

edited

Loading

lbenet Apr 6, 2018

tkoolen Apr 6, 2018

lbenet Apr 6, 2018

lbenet left a comment

tkoolen left a comment •

edited

Loading

tkoolen Apr 6, 2018

tkoolen commented Apr 6, 2018 •

edited

Loading

lbenet commented Apr 6, 2018

dpsanders commented Apr 9, 2018

tkoolen commented Apr 9, 2018

dpsanders commented Apr 9, 2018

lbenet commented Apr 9, 2018

tkoolen commented Apr 9, 2018

dpsanders commented Apr 10, 2018

Introduce atomic function, extracted out of convert #114

Introduce atomic function, extracted out of convert #114

Conversation

tkoolen commented Apr 6, 2018 • edited Loading

codecov-io commented Apr 6, 2018 • edited Loading

Codecov Report

coveralls commented Apr 6, 2018

coveralls commented Apr 6, 2018 • edited Loading

lbenet Apr 6, 2018

Choose a reason for hiding this comment

tkoolen Apr 6, 2018

Choose a reason for hiding this comment

lbenet Apr 6, 2018

Choose a reason for hiding this comment

lbenet left a comment

Choose a reason for hiding this comment

tkoolen left a comment • edited Loading

Choose a reason for hiding this comment

tkoolen Apr 6, 2018

Choose a reason for hiding this comment

tkoolen commented Apr 6, 2018 • edited Loading

lbenet commented Apr 6, 2018

dpsanders commented Apr 9, 2018

tkoolen commented Apr 9, 2018

dpsanders commented Apr 9, 2018

lbenet commented Apr 9, 2018

tkoolen commented Apr 9, 2018

dpsanders commented Apr 10, 2018

Introduce `atomic` function, extracted out of `convert` #114

Introduce `atomic` function, extracted out of `convert` #114

tkoolen commented Apr 6, 2018 •

edited

Loading

codecov-io commented Apr 6, 2018 •

edited

Loading

coveralls commented Apr 6, 2018 •

edited

Loading

tkoolen left a comment •

edited

Loading

tkoolen commented Apr 6, 2018 •

edited

Loading