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

Fix bug in check_method_implementation #169

Merged
merged 3 commits into from
Jan 27, 2018
Merged

Fix bug in check_method_implementation #169

merged 3 commits into from
Jan 27, 2018

Conversation

schillic
Copy link
Member

@schillic schillic commented Jan 21, 2018

@schillic
Copy link
Member Author

@mforets: How should we implement vertices_list?
One idea: Implement the intersection for two Hyperplanes (result: a Hyperplane of lower dimension). Then intersect all defining hyperplanes of the polytope as long as possible until you arrive at 1D. Is this necessary for every (unordered) pair of constraints?

@mforets mforets mentioned this pull request Jan 25, 2018
@mforets
Copy link
Member

mforets commented Jan 26, 2018

does the vertices list functionality depend on the "check_method_implementation"? how does that work?

@mforets mforets changed the title [WIP #168] vertices_list for HPolytope Vertices_list for HPolytope Jan 26, 2018
@mforets mforets changed the title Vertices_list for HPolytope Fix bug in check_method_implementation Jan 26, 2018
@schillic
Copy link
Member Author

It just checks if the method exists. When #189 is merged, it should work (we should still try).

@mforets
Copy link
Member

mforets commented Jan 26, 2018

do you refer to this test?

@test check_method_implementation(AbstractPolytope, vertices_list,
                                  Function[S -> (S{Float64},)])

@schillic
Copy link
Member Author

Yes. I just rebased with master. If it gets green, you can merge.

@mforets
Copy link
Member

mforets commented Jan 26, 2018

the access of the method vertices_list requires Polyhedra (it's hidden under a @require macro, since we don't want it to be loaded at startup). Maybe we needs using Polyhedra before the test of this file?

@schillic
Copy link
Member Author

Indeed:

julia> method_exists(vertices_list, (HPolytope{Float64},))
false

julia> using Polyhedra

julia> method_exists(vertices_list, (HPolytope{Float64},))
true

@schillic
Copy link
Member Author

schillic commented Jan 27, 2018

Unfortunately, when adding using Polyhedra, I get an error with Halfspace, see #194.

LoadError: UndefVarError: HalfSpace not defined

@schillic
Copy link
Member Author

schillic commented Jan 27, 2018

For now my fix is to load this test set last. We should still investigate #194 another time.

Another solution would be to provide a default implementation outside the @require which just throws an error. But then there might be a "redefinition" warning every time.

@schillic schillic merged commit 6496a5e into master Jan 27, 2018
@schillic schillic deleted the schillic/168 branch January 27, 2018 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants