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
Qualify the use of NeuralVerification.jl's "complement" function #190
Conversation
Instead of this patch, we should actually be able to adopt
but otherwise it seems all of the same operations should work |
Okay that's a good idea, I'll work on that refactor now! |
@tomerarnon One interesting bit of behavior from the LazySets implementation:
The first will print false while the second prints true. So even though the left and right of the first equals check are the same geometric object, they don't get evaluated as equal right now. Will this break anything? I added an issue on LazySets JuliaReach/LazySets.jl#2686 to see if this is a bug or if it's expected behavior. |
@@ -208,8 +208,8 @@ function add_complementary_set_constraint!(m::Model, H::HalfSpace, z::Vector{Var | |||
@constraint(m, a * z .>= b) | |||
return nothing | |||
end | |||
function add_complementary_set_constraint!(m::Model, PC::PolytopeComplement, z::Vector{VariableRef}) | |||
add_set_constraint!(m, PC.P, z) | |||
function add_complementary_set_constraint!(m::Model, PC::Complement{<:Real, <:AbstractPolytope}, z::Vector{VariableRef}) |
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.
Can we go with just Complement
? There shouldn't be a need for the type parameters.
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.
Yeah, I was worried we would need some way to make sure it's the complement of a polytope. Eventually we convert it to a matrix representation, so if it were the complement of an L-2 Ball or something we wouldn't want to let it in. Is there some less restrictive way to ensure it will be convertible to hrep?
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 there some less restrictive way to ensure it will be convertible to hrep?
we usually use applicable(constraints_list, X)
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.
Also, since the functions that use Complement
just unwrap it and pass to the complementary function (which are all strictly typed (HalfSpace, HPolytope, etc.), we will just run into the same method error one layer deeper, which is fine. So we should be able to stick to ::Complement
without a problem.
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.
oh got it, then typing to Complement
alone should address the problem right? I just updated it to be like that and have re-requested review.
Yes, this is a bit annoying. It's just because LazySets hasn't defined I'm not sure whether we check equality of Complements anywhere 😳 |
Yeah I also can't think of anywhere we use that equality. For the most part, we seem to use it to wrap up a polytope, and eventually that polytope gets unwrapped and turned into linear constraints, or its intersection / other set type operations are performed on it, both of which should still work fine without the equality operator. |
The complement function was breaking Travis builds for Julia 1.5 at least, where when complements.jl tests the complement function it wasn't sure whether it referred to LazySets.complement or NeuralVerification.complement. I added on NeuralVerification.jl to all uses of complement in that file.