-
Notifications
You must be signed in to change notification settings - Fork 32
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
#92 - Better calculation of list of vertices for degenerate centrally symmetric polytopes #590
Conversation
src/AbstractHyperrectangle.jl
Outdated
@@ -51,6 +51,10 @@ For high dimensions, it is preferable to develop a `vertex_iterator` approach. | |||
""" | |||
function vertices_list(H::AbstractHyperrectangle{N} | |||
)::Vector{Vector{N}} where {N<:Real} | |||
# fast evaluation if B has radius 0 | |||
if contains(==, radius_hyperrectangle(H), zero(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.
I do not understand this check. I would expect
vertices_list(Hyperrectangle([1., 1.], [0., 1.]) == [[1., 0.], [1., 2.]]
.
For me, the check should be radius_hyperrectangle(H) == zeros(N, dim(H))
.
(The unit test has to be adapted accordingly.)
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.
If this was referring to this comment, I meant that the result should contain less than 2^n vertices.
test/unit_Hyperrectangle.jl
Outdated
# check that vertices_list is computed correctly if the hyperrectangle | ||
# is "degenerate" in the sense that its radius is zero in at least one dimension | ||
# this test would take very long if all 2^100 vertices are computed (see #92) | ||
H = Hyperrectangle(fill(N(1.), 100), [fill(N(2.), 99); 0.]) |
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.
Apparently, you need to write N(0.)
again. Otherwise it will make the whole vector a Float64
.
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.
This fixes my concerns.
The efficiency can still be improved if the radius is 0 for some dimensions, but we can do that later.
EDIT: I created #593.
thanks for the feedback! |
Should we add a unit test for the |
Closes #92.