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

#1272 - Use eps by default in plots #1303

Merged
merged 19 commits into from
May 2, 2019
Merged

#1272 - Use eps by default in plots #1303

merged 19 commits into from
May 2, 2019

Conversation

mforets
Copy link
Member

@mforets mforets commented Apr 22, 2019

Closes #1272.
Closes #578.
Closes #575.

With this change, the manual has to be updated accordingly (in several places passing the epsilon explicitly is no longer needed). We can do this in a follow-up PR -- this is now #1321.

@mforets mforets requested a review from schillic April 22, 2019 18:41
@mforets mforets changed the title #1272 - Use eps by default in plots #1272 - WIP Use eps by default in plots Apr 22, 2019
Copy link
Member

@schillic schillic left a comment

Choose a reason for hiding this comment

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

src/plot_recipes.jl Outdated Show resolved Hide resolved
src/plot_recipes.jl Outdated Show resolved Hide resolved
@mforets
Copy link
Member Author

mforets commented Apr 24, 2019

Why remove the tests for Rational{Int} and Float32?

I was planning to put Float32 back after generalizing the epsilon threshold to N. Note, however that the tests don't pass now, because of missing docstrings (how to fix this?).

About Rational{Int} i'm not sure that it even makes sense to apply iterative refinement, since 2-norms etc are used in the algorithm etc (one would have to check carefully that the operations make sense and take overapproximations, like overapproximation of those norms using rational, etc.). As a consequence, even for "simple" lazy sets the refinement fails if you pass rationals:

# the test below also fails if you take X = Ball1([0//1, 0//1], 1//1), but if you take vertices_list(X) works 
# so it can be plotted

julia> X = Diagonal([1//1, 1//1]) * Ball1([0//1, 0//1], 1//1)
LinearMap{Rational{Int64},Ball1{Rational{Int64}},Rational{Int64},Diagonal{Rational{Int64},Array{Rational{Int64},1}}}(Rational{Int64}[1//1 0//1; 0//1 1//1], Ball1{Rational{Int64}}(Rational{Int64}[0//1, 0//1], 1//1))

julia> overapproximate(X, 1//10)
ERROR: MethodError: no method matching σ(::Array{Float64,1}, ::LinearMap{Rational{Int64},Ball1{Rational{Int64}},Rational{Int64},Diagonal{Rational{Int64},Array{Rational{Int64},1}}})
Closest candidates are:
  σ(::AbstractArray{N<:Real,1}, ::HalfSpace{N<:Real}) where N<:Real at /Users/forets/.julia/dev/LazySets/src/HalfSpace.jl:110
  σ(::AbstractArray{N<:Real,1}, ::ZeroSet{N<:Real}) where N<:Real at /Users/forets/.julia/dev/LazySets/src/ZeroSet.jl:98
  σ(::AbstractArray{N<:Real,1}, ::AbstractSingleton{N<:Real}) where N<:Real at /Users/forets/.julia/dev/LazySets/src/AbstractSingleton.jl:231
  ...
Stacktrace:
 [1] new_approx(::LinearMap{Rational{Int64},Ball1{Rational{Int64}},Rational{Int64},Diagonal{Rational{Int64},Array{Rational{Int64},1}}}, ::Array{Rational{Int64},1}, ::Array{Rational{Int64},1}, ::Array{Rational{Int64},1}, ::Array{Rational{Int64},1}) at /Users/forets/.julia/dev/LazySets/src/Approximations/iterative_refinement.jl:95
 [2] addapproximation!(::LazySets.Approximations.PolygonalOverapproximation{Rational{Int64}}, ::Array{Rational{Int64},1}, ::Array{Rational{Int64},1}, ::Array{Rational{Int64},1}, ::Array{Rational{Int64},1}) at /Users/forets/.julia/dev/LazySets/src/Approximations/iterative_refinement.jl:124
 [3] approximate(::LinearMap{Rational{Int64},Ball1{Rational{Int64}},Rational{Int64},Diagonal{Rational{Int64},Array{Rational{Int64},1}}}, ::Rational{Int64}) at /Users/forets/.julia/dev/LazySets/src/Approximations/iterative_refinement.jl:219
 [4] overapproximate(::LinearMap{Rational{Int64},Ball1{Rational{Int64}},Rational{Int64},Diagonal{Rational{Int64},Array{Rational{Int64},1}}}, ::Type{HPolygon}, ::Rational{Int64}) at /Users/forets/.julia/dev/LazySets/src/Approximations/overapproximate.jl:51
 [5] overapproximate(::LinearMap{Rational{Int64},Ball1{Rational{Int64}},Rational{Int64},Diagonal{Rational{Int64},Array{Rational{Int64},1}}}, ::Rational{Int64}) at /Users/forets/.julia/dev/LazySets/src/Approximations/overapproximate.jl:63
 [6] top-level scope at none:0

@schillic
Copy link
Member

About Rational{Int} i'm not sure that it even makes sense to apply iterative refinement

No, but we can just move these tests to the other loop without Rational{Int}. We can still plot AbstractPolytopes as before.

@mforets
Copy link
Member Author

mforets commented Apr 24, 2019

Some of those tests in Rational{Int} of the outer-most loop worked just because the previous code was creating box overapproximations by default; note that overapproximate(X, Inf) in the previous example does work if the coeffs of X are rationals, but it breaks whenever you want to refine the box.

@schillic
Copy link
Member

That is why I said we can move those tests to the other loop.

@mforets
Copy link
Member Author

mforets commented Apr 24, 2019

In unit_plot.jl, we can now remove the tests with a custom epsilon, since iterative refinement is used anyways if you plot(X).

@schillic
Copy link
Member

We could still test whether passing the value works, but I agree.

@mforets mforets changed the title #1272 - WIP Use eps by default in plots #1272 - Use eps by default in plots May 1, 2019
docs/src/lib/interfaces.md Outdated Show resolved Hide resolved
test/unit_plot.jl Outdated Show resolved Hide resolved
src/plot_recipes.jl Outdated Show resolved Hide resolved
src/plot_recipes.jl Outdated Show resolved Hide resolved
src/plot_recipes.jl Outdated Show resolved Hide resolved
src/plot_recipes.jl Outdated Show resolved Hide resolved
src/plot_recipes.jl Outdated Show resolved Hide resolved
src/plot_recipes.jl Outdated Show resolved Hide resolved
src/plot_recipes.jl Outdated Show resolved Hide resolved
src/plot_recipes.jl Outdated Show resolved Hide resolved
Copy link
Member

@schillic schillic left a comment

Choose a reason for hiding this comment

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

Maybe we should have a global constant (e.g., PLOT_PRECISION) for 1e-3. Or even just a variable so it can be set from outside.

@mforets mforets merged commit d985026 into master May 2, 2019
@mforets mforets deleted the mforets/1272 branch May 2, 2019 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants