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

Fix Stackoverflow when creating interval of intervals

Open Kolaru opened this issue 7 years ago • 11 comments

Fix #182. I have define a new type alias of all supported type (basically all built-in number type). Therefore this make the implementation the less generic possible, which is also the safest.

The change also somehow confuse Julia when it comes to finding the most specific method in some cases, for unknown reason.

Finally a small change in the display function to be sure to avoid an infinite recursion by using specifically the BigFloat constructor.

Kolaru avatar Jul 20 '18 22:07 Kolaru

Coverage Status

Coverage remained the same at 92.444% when pulling 43a10732e226d0222f4cb27fb3b905e5193fa0a9 on Kolaru:fix_display_recursion into cab2433d6719899c38c0c3514fc935208614bdd0 on JuliaIntervals:master.

coveralls avatar Jul 20 '18 23:07 coveralls

Coverage Status

Coverage decreased (-0.07%) to 90.958% when pulling 337ce67cfa35fd1d95ac68c75a6ebb4b2c19d084 on Kolaru:fix_display_recursion into 4d0eba52b8f8cd883f7b6c9b3f4cd06c4c22d0b7 on JuliaIntervals:master.

coveralls avatar Jul 20 '18 23:07 coveralls

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@1516c5e). Click here to learn what that means. The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #185   +/-   ##
=========================================
  Coverage          ?   82.31%           
=========================================
  Files             ?       25           
  Lines             ?     1227           
  Branches          ?        0           
=========================================
  Hits              ?     1010           
  Misses            ?      217           
  Partials          ?        0
Impacted Files Coverage Δ
src/display.jl 80.61% <ø> (ø)
src/intervals/arithmetic.jl 94.11% <ø> (ø)
src/intervals/intervals.jl 87.23% <88.88%> (ø)

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 1516c5e...b4fbd97. Read the comment docs.

codecov-io avatar Jul 20 '18 23:07 codecov-io

Thanks @Kolaru for looking into this. I just checked in Julia 1.0 and the problem reported in #182 persists.

I like your idea of introducing IASupportedType here. However, your approach may be too restrictive, in the sense that a user may introduce another struct which is subtype of Real, for which it makes sense to define intervals; an example could be DoubleDouble or ArbFloat.

I think the following would allow for such an enlargement, but I haven't being able to test it in Julia 1.0.

using InteractiveUtils: subtypes
const IASupportedType = Union{setdiff(subtypes(Real), [AbstractInterval])...}

This would result on IASupportedType being Union{AbstractIrrational, AbstractFloat, Integer, Rational}, which is essentially what you listed.

Another option, as @dpsanders suggested, is that Interval(::Interval) throws an error.

lbenet avatar Sep 27 '18 16:09 lbenet

What about a trait-like solution ? Something like

struct Interval{T} where {T <: Real}
  a::T
  b::T
  Interval{T}(::Type{Val{true}}, a::T, b::T) = new(a, b)
  Interval{T}(::Type{Val{false}}, ::T, ::T) = error("Type $T not good for interval arithmetic.")
end

Interval(a::T, b::T) where {T <: Real} = Interval(Val{isGoodForIA(T)}, a, b)

So to make a custom type A usable in interval, the user would simply have to write

isGoodForIA(A) = true

Like that absurd intervals are never passed silently, but it can still easily be extended with arbitrary type. Also I don't think this would justifiy adding SimpleTraits.jl as dependency, since it is quite simple still.

Kolaru avatar Sep 28 '18 15:09 Kolaru

I closed the wrong PR, sorry for this.

Kolaru avatar Mar 27 '19 22:03 Kolaru

What's the status here? Should we merge this?

dpsanders avatar Jun 24 '19 15:06 dpsanders

I need to go back to it to address your last review. I don't remember why I didn't do it earlier, but I'll do it shortly.

Kolaru avatar Jun 28 '19 00:06 Kolaru

The coverage decrease comes from the fact that can_bound_interval is not found to be hit by Coveralls, probably because the method is always optimized out by the compiler. I don't think that makes much sense to test it directly, so this PR should be ready.

Kolaru avatar Aug 16 '19 01:08 Kolaru

This is ready (and may also help avoid some amiguity error I get in #271 when trying to have generic constructors).

Kolaru avatar Oct 26 '20 00:10 Kolaru

Codecov Report

Merging #185 (09a4036) into master (a3dc27f) will decrease coverage by 0.02%. The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #185      +/-   ##
==========================================
- Coverage   91.55%   91.52%   -0.03%     
==========================================
  Files          25       25              
  Lines        1752     1758       +6     
==========================================
+ Hits         1604     1609       +5     
- Misses        148      149       +1     
Impacted Files Coverage Δ
src/intervals/arithmetic.jl 97.20% <50.00%> (ø)
src/intervals/intervals.jl 91.42% <92.30%> (-0.76%) :arrow_down:
src/IntervalArithmetic.jl 100.00% <100.00%> (ø)
src/display.jl 88.81% <100.00%> (ø)

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 a3dc27f...09a4036. Read the comment docs.

codecov-commenter avatar Jun 12 '21 01:06 codecov-commenter