-
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
#162 - Add macros to automatically create boilerplate code #228
Conversation
Example: Let us define some nonsense: julia> l = LinearConstraint([1.], 1.); e = EmptySet{Float64}();
julia> @commutative_neutral(LinearConstraint, EmptySet);
julia> LinearConstraint(l,e)
LazySets.LinearConstraint{Float64}([1.0], 1.0)
julia> LinearConstraint(e,l)
LazySets.LinearConstraint{Float64}([1.0], 1.0)
julia> LinearConstraint(e,e)
LazySets.EmptySet{Float64}()
julia> LinearConstraint(l,l)
ERROR: MethodError: no method matching LazySets.LinearConstraint(::LazySets.LinearConstraint{Float64}, ::LazySets.LinearConstraint{Float64}) |
ede59cd
to
772504d
Compare
3e1fa56
to
9348d4a
Compare
src/helper_functions.jl
Outdated
@@ -1,6 +1,8 @@ | |||
export sign_cadlag, | |||
check_method_implementation, | |||
check_method_ambiguity_binary | |||
check_method_ambiguity_binary, | |||
@commutative_neutral, |
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.
i'm not sure that we want to export these names, since this is more of an internals of the package (same comment applies to the 2 check_...
methods of above)
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 point.
But I think we need to export the check methods for usage in the unit tests (not tested). Somewhere I proposed that we remove them from the code base and move them to the unit tests. Any opinion?
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.
even if it is the name is not exported, one can use the function by writing the module's name, as in LazySets.check_method_implementation
, or it's used in many places do a import LazySets.check_method_implementation
.
but i agree to move those check methods to the tests folder 👍
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.
one can use the function by writing the module's name
You are right, I forgot that.
but i agree to move those check methods to the tests folder
Good, I changed #197 accordingly.
src/helper_functions.jl
Outdated
|
||
### Examples | ||
|
||
`@commutative_neutral(ConvexHull, EmptySet)` creates the following functions: |
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.
you can more synthetically write the equations, eg. "it creates the following functions for any set X
: CH(X, ∅) = CH( ∅, X) = X
and the edge caseCH(∅, ∅) = ∅
.
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.
∅
might be misleading, but I get your point.
src/helper_functions.jl
Outdated
|
||
### Examples | ||
|
||
`@commutative_neutral_absorbing(MinkowskiSum, ZeroSet, EmptySet))` creates the |
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.
extra parentheses after EmptySet
👁
|
||
- `CartesianProductArray()` -- constructor for an empty Cartesian product | ||
Constructors: |
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.
while we are at this line, a general thought is that i find the illustration of the different constructors to be more friendly if it is given as a proper example, rather than a copy paste of the source code -- if one wants that level of detail, then it's better to just look the code, by clicking on "view source", no? (or use Julia's @edit
macro). that is to say that for me we can delete this part and instead put some examples of the different uses (in another PR).
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.
I added #232.
function *(S::LazySet, cpa::CartesianProductArray)::CartesianProductArray | ||
push!(cpa.array, S) | ||
return cpa | ||
function CartesianProductArray(S::LazySet, |
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, this change is to save the symbol *
for the binary operator only, in other words these two that used to be the same are now different:
julia> b = Ball2(zeros(2), 1.);
julia> c = CartesianProductArray([b, b])
julia> CartesianProductArray(b, c)
LazySets.CartesianProductArray{Float64,LazySets.Ball2{Float64}}(LazySets.Ball2{Float64}[LazySets.Ball2{Float64}([0.0, 0.0], 1.0), LazySets.Ball2{Float64}([0.0, 0.0], 1.0), LazySets.Ball2{Float64}([0.0, 0.0],
1.0)])
julia> b * c
LazySets.CartesianProduct{Float64,LazySets.Ball2{Float64},LazySets.CartesianProductArray{Float64,LazySets.Ball2{Float64}}}(LazySets.Ball2{Float64}([0.0, 0.0], 1.0), LazySets.CartesianProductArray{Float64,Laz
ySets.Ball2{Float64}}(LazySets.Ball2{Float64}[LazySets.Ball2{Float64}([0.0, 0.0], 1.0), LazySets.Ball2{Float64}([0.0, 0.0], 1.0), LazySets.Ball2{Float64}([0.0, 0.0], 1.0)]))
i doubt a bit, but maybe it is more sensible the new behavior.
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.
Actually I wanted to discuss this.
Previously *
and CartesianProduct
were not identical. I found this inconsistent, because *(S, T)
was said to be an alias for CartesianProduct(S, T)
.
Now it is identical, but we get what you observed.
I propose to additionally add a similar macro relating CartesianProduct
and CartesianProductArray
(and similarly for the other two) such that we also get the old behavior.
If you agree, should I do it here or in a follow-up PR?
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.
follow-up PR
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.
Okay, I took a note in the related issue #162.
src/helper_functions.jl
Outdated
* `MinkowskiSum(::ZeroSet{N}, Y::EmptySet{N}) where {N<:Real} = Y` | ||
* `MinkowskiSum(Y::EmptySet{N}, ::ZeroSet{N}) where {N<:Real} = Y` | ||
""" | ||
macro commutative_neutral_absorbing(SET, NEUT, ABS) |
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.
i was wondering if it's possible to work it around such that the extra work done by commutative_neutral_absorbing
is done on the function commutative_absorbing
, provided that the neutral element of the given set has been defined. we can do this by adding a function neutral(::set_type)::neutral_type
and a check isdefined(neutral($SET))
.
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.
Hm, we have three of these cases, one with only neutral, one with only absorbing, and one with both neutral and absorbing. So I do not see why neutral
should play a different role.
But I like the general idea. I would rather add two functions neutral
and absorbing
as follows.
The default implementation is neutral(set) = nothing
and for the respective types we override with neutral(MinkowskiSum) = ZeroSet
. Then we can get rid of two of the three macros. What would be the name of the remaining macro then?
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.
Then we can get rid of two of the three macros.
how is that? i had in mind to substitute the call to commutative_neutral_absorbing
to the calls commutative_neutral
and commutative_absorbing
. but one wants to do this such that the order of the calls doesn't matter :/
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.
You just call this new function (call it fun
) and internally it calls the neutral
and absorbing
functions to see what it should do. Does it make sense?
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.
I understood now that what you proposed was very similar, sorry for the confusion.
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.
with only 1 macro, the name commutative_neutral_absorbing
is not the most intuitive IMO since EmptySet
has nothing to do with _neutral_
of CartesianProduct
, while we have to write:
# EmptySet is the absorbing element for CartesianProduct
absorbing(::Type{CartesianProduct}) = EmptySet
# create absorbing element functions
@commutative_neutral_absorbing(CartesianProduct)
idea:
# EmptySet is the absorbing element for CartesianProduct
@absorbing(CartesianProduct) = EmptySet
(or statement like: Emptyset @absorbs CartesianProduct
or @is_absorbing_for
)
that unrolls to absorbing(::Type{CartesianProduct}) = EmptySet
and the function definitions macro. but for a case like M-sum one does:
@absorbing(MinkowskiSum) = EmptySet
@neutral(MinkowskiSum) = ZeroSet
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.
I admit that this is a very nice solution from the reader's perspective. Note that we will not have documentation for those generated functions, but they are anyway not interesting. Let me see if it works out 👍
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.
@absorbing(MinkowskiSum) = EmptySet
I think you cannot do assignments in macros. So I will introduce @absorbing(MinkowskiSum, EmptySet)
Ready from my side. |
I updated again. Side note: This macro stuff can be complicated at times. In the end I used a bit of copy-paste because I lost patience. |
docs/src/lib/utils.md
Outdated
@@ -9,4 +9,7 @@ end | |||
|
|||
```@docs | |||
sign_cadlag | |||
@neutral | |||
@absorbing | |||
@neutral_and_absorbing |
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.
is @neutral_and_absorbing
a remainder?
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.
oops, fixed
ef0e89e
to
c69313d
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.
nice work with the macros 👌
Thanks for the help and reviews, I think it is much better now. |
See #162.
Also resolves most of #145 (had to be done in one PR) (only
ConvexHull
missing).