-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add polynomial zonotope #184
Conversation
src/PolynomialZonotope.jl
Outdated
|
||
Abstract type for non-convex sets. | ||
""" | ||
abstract type AbstractNonConvexSet{N} end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let us do this properly in a separate PR:
- Move to a new file (and maybe rename to
AbstractSet
or evenSet
orLazySet
).¹ - Make
LazySet
inherit from this new type and rename toAbstractConvexSet
or justConvexSet
.¹ This basically means changing every file, so better merge most PRs first. - Make
UnionSet
inherit from this new type and include it (there are already some unit tests).
Since this is a breaking change, we should probably do it before releasing 1.1.0.
¹This will be something we and the user will write a lot, so a short name would be preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
outsourced to #185
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a breaking change, we should probably do it before releasing 1.1.0.
2.0 iiuc since that would be an incompatible API change (https://semver.org/)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok i've updated to <: LazySet{N}
, what i should have done in the 1st place, at least 'till we resolve the approach for #185.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#185 was created for this issue (only later). You could not have done that before ;)
src/PolynomialZonotope.jl
Outdated
|
||
- `c` -- starting point | ||
- `E` -- multi-indexed generators such that *all* indices have the same value | ||
- `F` -- multi-indexed generators such that *not all* indices are the same value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"are" → "have"
src/PolynomialZonotope.jl
Outdated
|
||
### Notes | ||
|
||
Polynomial zonotope were introduced by M. Althoff in *Reachability analysis of nonlinear |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"zonotope" → "zonotopes"
src/PolynomialZonotope.jl
Outdated
abstract type AbstractNonConvexSet{N} end | ||
|
||
""" | ||
PolynomialZonotope{N<:Real} <: AbstractNonConvexSet{N} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you have N<:Real
, but not in the actual struct definition.
src/PolynomialZonotope.jl
Outdated
|
||
- `pz` -- polynomial zonotope | ||
""" | ||
dim(pz::PolynomialZonotope) = length(pz.c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add output docs and output type?
src/PolynomialZonotope.jl
Outdated
The polynomial order, defined as the maximal power of the scalar factors ``β_i``. | ||
Usually denoted ``η``. | ||
""" | ||
polynomial_order(pz::PolynomialZonotope) = length(pz.E) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add output type?
src/PolynomialZonotope.jl
Outdated
|
||
## Output | ||
|
||
The polynomial order, defined as the number of generators divided by the ambient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop "polynomial".
src/PolynomialZonotope.jl
Outdated
The polynomial order, defined as the number of generators divided by the ambient | ||
dimension. | ||
""" | ||
function order(pz::PolynomialZonotope) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add output type?
To-do:
|
9b66fb1
to
c9a5be6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, although I did not check the maths :)
src/PolynomialZonotope.jl
Outdated
|
||
### Input | ||
|
||
- `α` -- polynomial zonotope |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scalar factor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
## Output | ||
|
||
Polynomial zonotope such that its center and generators are multiples of those |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above you called the center "starting point".
|
||
Polynomial zonotope. | ||
""" | ||
function minkowski_sum(pz::PolynomialZonotope, z::Zonotope) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the symmetric method? It can fall back to this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good. i imagine some macro for lazy people (like us) used as:
@symmetric function minkowski_sum(pz::PolynomialZonotope, z::Zonotope)
c = pz.c + z.center
G = [G z.generators]
return PolynomialZonotope(c, pz.E, pz.F, G)
end
and builds the new symmetric function using this one.
|
||
# type-specific concrete methods | ||
scale(N(0.5), p) | ||
linear_map(N[1.0 2.0; 2.0 5.0], p) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a unit test for minkowski_sum
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. i plan to add proper @test
's when with #230 gets in
I added "support vector" to the open tasks list above - do not forget it ;) |
cc: @nikos-kekatos