added constraints_list for CartesianProduct #761

kpotomkin · 2018-10-10T14:53:51Z

Closes #753.

schillic · 2018-10-10T15:01:23Z

src/CartesianProduct.jl

+    # create high-dimensional constraints list
+    for X in clist_low
+        for constr in X
+            new_constr = LinearConstraint(sparsevec(prev_step : (dim(constr) + prev_step-1), constr.a), constr.b)


The indices are a constant vector for each X, so I would pull it out of the inner loop:

if !isempty(X) indices = prev_step : (dim(X[1]) + prev_step-1) end for constr in X new_constr = LinearConstraint(sparsevec(indices, constr.a), constr.b) ...

schillic · 2018-10-10T15:04:39Z

src/CartesianProduct.jl

+    sizehint!(clist, m)
+    prev_step = 1
+    # create high-dimensional constraints list
+    for X in clist_low


Maybe do not use X; we usually use that for sets.

Since there is no difference between code base for CartesianProduct and CartesianProductArray I guess it makes sense to merge this code. However, I am not sure how bad this solution for performance (in other words how expensive is creating CartesianProductArray)

schillic

Efficiency remark: Do not create a big array with all constraints that you only run through again in the end. Immediately create the new constraints.

schillic · 2018-10-10T19:41:52Z

src/CartesianProduct.jl

+            indices = prev_step : (dim(c_low[1]) + prev_step - 1)
+        end
+        for constr in c_low
+            new_constr = LinearConstraint(sparsevec(prev_step : (dim(constr) + prev_step-1), constr.a), constr.b)


Use the indices you defined above 😄

mforets · 2018-10-10T19:55:36Z

src/CartesianProduct.jl

+A list of constraints.
+
+"""
+function constraints_list(cpa::CartesianProductArray{N})::Vector{LinearConstraint{N}} where N<:Real


I think we should better use the interface constraints_list(cpa::CartesianProductArray{N, <:AbstractPolytope} given the recent introduction of constraints_list list method for the whole interface.

There is a drawback in this approach (but i don't have a clear picture how to make this play nicely with interfaces), in that the interface restriction i propose will make constraints_list of, say, the lazy linear map of a polytope not work. But first we should add constraints_list(::LazySets.LinearMap{Float64,LazySets.HPolytope{Float64},Float64,Array{Float64,2}}) anyways.

mforets · 2018-10-10T20:11:00Z

src/CartesianProduct.jl

+"""
+function constraints_list(cpa::CartesianProductArray{N})::Vector{LinearConstraint{N}} where N<:Real
+    # collect low-dimensional constraints lists
+    clist_low = []


clist_low = [] is an array of type Any. Passing the information known to the compiler will improve efficiency, basically a clist_low = Vector{LinearConstraint{N}}(). See type-stability performance notes.

schillic

Now we need the docs and a unit test for each function.
The docs go to docs/src/lib/operations.md and then in the respective section add
constraints_list(::CartesianProduct{N, <:AbstractPolytope}) where N<:Real
resp.
constraints_list(::CartesianProductArray{N, <:AbstractPolytope}) where N<:Real.
The unit tests go to test/unit_CartesianProduct.jl. A simple test with two Intervals is sufficient. But please write them parametric in N (the numeric type).

schillic · 2018-10-11T06:33:51Z

src/CartesianProduct.jl

+
+"""
+function constraints_list(cpa::CartesianProductArray{N, <:AbstractPolytope})::Vector{LinearConstraint{N}} where N<:Real
+    # collect low-dimensional constraints lists


redundant now

schillic · 2018-10-11T06:34:20Z

src/CartesianProduct.jl

+### Output
+
+A list of constraints.
+


nit-picky: empty line

schillic · 2018-10-11T06:36:05Z

src/CartesianProduct.jl

@@ -151,6 +151,23 @@ function ∈(x::AbstractVector{<:Real}, cp::CartesianProduct)::Bool
           ∈(view(x, n1+1:length(x)), cp.Y)
 end

+"""
+    constraints_list(cp::CartesianProduct{N})::Vector{LinearConstraint{N}} where N<:Real


update (copy the function header here (without function))

schillic · 2018-10-11T06:36:16Z

src/CartesianProduct.jl

@@ -353,6 +370,42 @@ function ∈(x::AbstractVector{N}, cpa::CartesianProductArray{N, <:LazySet{N}}
    return true
 end

+"""
+    constraints_list(cpa::CartesianProductArray{N})::Vector{LinearConstraint{N}} where N<:Real


mforets · 2018-10-11T11:30:15Z

src/CartesianProduct.jl

+function constraints_list(cpa::CartesianProductArray{N, <:AbstractPolytope})::Vector{LinearConstraint{N}} where N<:Real
+    c_array = array(cpa)
+    clist = Vector{LinearConstraint{N}}()
+    sizehint!(clist, sum(dim(s_low) for s_low in c_array))


note that dim(cpa) exists too

schillic · 2018-10-11T16:01:25Z

src/CartesianProduct.jl

@@ -391,10 +391,10 @@ function constraints_list(cpa::CartesianProductArray{N, <:LazySet{N}})::Vector{L
    for c_low in array(cpa)
        c_low_list = constraints_list(c_low)
        if !isempty(c_low_list)
-            indices = prev_step : (dim(c_low_list[1]) + prev_step - 1)
+            indices = collect(prev_step : (dim(c_low_list[1]) + prev_step - 1))


What was bad about the colon notation? It was more efficient...

mforets · 2018-10-11T19:11:55Z

@kostakoida feel free to add your name in https://juliareach.github.io/LazySets.jl/latest/about.html#Credits-1

added constraints_list for CartesianProduct

bf12e95

schillic reviewed Oct 10, 2018

View reviewed changes

kpotomkin added 2 commits October 11, 2018 02:29

added constraints_list for CartesianProductArray

cfb077c

removed redundant code

90e59d1

Since there is no difference between code base for CartesianProduct and CartesianProductArray I guess it makes sense to merge this code. However, I am not sure how bad this solution for performance (in other words how expensive is creating CartesianProductArray)

kpotomkin self-assigned this Oct 10, 2018

schillic reviewed Oct 10, 2018

View reviewed changes

mforets reviewed Oct 10, 2018

View reviewed changes

kpotomkin added 2 commits October 11, 2018 14:44

refactored constraints_list for CartesianProduct

cac3787

Updated method signature

ab691af

schillic reviewed Oct 11, 2018

View reviewed changes

small changes in comments sections

02e23ed

mforets reviewed Oct 11, 2018

View reviewed changes

kpotomkin added 2 commits October 12, 2018 01:33

add docs, tests, update signature of the method

23f2d6e

fixed wrong dimension in sparsevec

ae15a3e

schillic reviewed Oct 11, 2018

View reviewed changes

kpotomkin and others added 3 commits October 12, 2018 03:42

fixed wrong dimension

07f512e

Merge remote-tracking branch 'origin/master' into 753

3e1b468

minor changes

720a5ad

schillic approved these changes Oct 11, 2018

View reviewed changes

schillic merged commit 7ce4dc0 into master Oct 11, 2018

schillic deleted the kostemkin/753 branch October 11, 2018 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added constraints_list for CartesianProduct #761

added constraints_list for CartesianProduct #761

kpotomkin commented Oct 10, 2018 •

edited by schillic

Loading

schillic Oct 10, 2018

schillic Oct 10, 2018 •

edited

Loading

schillic left a comment

schillic Oct 10, 2018

mforets Oct 10, 2018

mforets Oct 10, 2018

schillic left a comment •

edited

Loading

schillic Oct 11, 2018

schillic Oct 11, 2018

schillic Oct 11, 2018

schillic Oct 11, 2018

mforets Oct 11, 2018

schillic Oct 11, 2018

mforets commented Oct 11, 2018

added constraints_list for CartesianProduct #761

added constraints_list for CartesianProduct #761

Conversation

kpotomkin commented Oct 10, 2018 • edited by schillic Loading

Choose a reason for hiding this comment

schillic Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

schillic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schillic left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mforets commented Oct 11, 2018

kpotomkin commented Oct 10, 2018 •

edited by schillic

Loading

schillic Oct 10, 2018 •

edited

Loading

schillic left a comment •

edited

Loading