-
Notifications
You must be signed in to change notification settings - Fork 7
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 removevredundancy!
#53
Conversation
Tests should fail for this commit
You can update Line 34 in 22915a1
by pushing ineq + 1 and here Line 40 in 22915a1
You can replace ineq::Int by ineq::Polyhedra.Index .Then it's clear we are talking about 1-based indexing because of: Line 230 in 22915a1
|
@@ -55,8 +55,6 @@ function getine(p::Polyhedron) | |||
else | |||
p.ine = LiftedHRepresentation(getextm(p, :Fresh)) | |||
p.inem = nothing | |||
p.hlinearitydetected = true |
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.
Leave the setting of hlinearitydetected
here and add a mention to the issue to say that in some examples there are still redundant elements
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.
Changed back p.hlinearitydetected = true
and p.vlinearitydetected = true
in getine
and getext
.
(By the way, please feel free to edit this branch.)
Pushing |
@blegat Is it possible that for H-representation, the first column (row 0 in the C indexing) is used for the objective function, while it is not case for V-representation? (I.e., Does "row i" point to different rows between H- and V-representations?) Lines 51 to 54 in 22915a1
Lines 77 to 80 in 22915a1
|
Yes, and you always need to set an objective for the H-rep: Lines 51 to 54 in 22915a1
|
I think I understand the things now.
The bug can be fixed by either
Each of this PR and #54 is ready to merge. Please choose 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.
Thanks! Let's merge this one as it is non-breaking for redund
then we'll see if we want to additionally make a breaking change
Fix #52
removevredundancy!
removes wrong row #52 (comment)removevredundancy!
removes wrong row #52 (comment)Closes #50