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

Make VPolygon's vertices field be abstract #927

Closed
mforets opened this issue Nov 23, 2018 · 5 comments · Fixed by #1385
Closed

Make VPolygon's vertices field be abstract #927

mforets opened this issue Nov 23, 2018 · 5 comments · Fixed by #1385
Assignees
Labels
usability 🖱️ Simplifies the usage or interface

Comments

@mforets
Copy link
Member

mforets commented Nov 23, 2018

using LazySets, StaticVectors

julia> VPolygon([@SVector(rand(2)) for i in 1:3])
ERROR: MethodError: Cannot `convert` an object of type Array{SArray{Tuple{2},Float64,1,2},1} to an object of type VPolygon
Closest candidates are:
  convert(::Type{VPolygon}, ::AbstractHPolygon) at /Users/forets/.julia/dev/LazySets/src/convert.jl:65
  convert(::Type{VPolygon}, ::AbstractPolytope) at /Users/forets/.julia/dev/LazySets/src/convert.jl:119
  convert(::Type{T}, ::T) where T at essentials.jl:154
Stacktrace:
 [1] VPolygon(::Array{SArray{Tuple{2},Float64,1,2},1}) at ./deprecated.jl:473
 [2] top-level scope at none:0
@schillic
Copy link
Member

I do not understand the suggestion.

@mforets
Copy link
Member Author

mforets commented Nov 23, 2018

To change the field vertices::Vector{Vector{N}} to something like vertices::Vector{AbstractVector{N}} such that the points in the vpolygon can be represented by any abstract vector (but possibly concretely typed, so a new parameter should be added to the type),

@schillic schillic added the usability 🖱️ Simplifies the usage or interface label Nov 23, 2018
@schillic
Copy link
Member

Okay, I agree, but some comments:

  1. Why only generalize the inner vector type?
  2. Is there something special about VPolygon that we only want to generalize the type here? What about VPolytope for instance?
  3. I do not know if it makes sense to make this type visible as a type parameter. I cannot think of a use case where you want to dispatch on this type. So AbstractVector is sufficient from my side.

@mforets
Copy link
Member Author

mforets commented Nov 23, 2018

1

The outer container may be kept as a usual vector, no? But as in the example of above, you may want to change the container of the points. For instance it makes sense to use static vectors to represent many vectors in 2D or 3D, but it is not so suited for holding a large number of vectors (the outer container).

2

True, i think we can as well generalize VPolytope. Note that other types already work, eg. one can create a linear constraint whose constraint vector is given as a static array.

3

My concern was about avoiding fields with abstract type.

@schillic
Copy link
Member

The outer container may be kept as a usual vector, no?

Why? 😃
I can think of a case where you want to pass a SubArray (from "view") for instance. Or maybe even an iterator that creates the vertices on-the-fly.
But I understand that then we would have to change a lot of types.

not so suited for holding a large number of vectors

I do not expect that in 2D you have a large number anyway.

3

Okay, I was not sure anymore if it had impact on efficiency. In that case of course we should add a type parameter.

Note that at some places we may have the assumption that the type of points is Vector{N} (for instance in some return types) (but I did not check). This would have to be adapted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
usability 🖱️ Simplifies the usage or interface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants