Skip to content

Conversation

odow
Copy link
Member

@odow odow commented Oct 2, 2018

We need to loop through and collect upper and lower terms, otherwise we can end up trying to set
variable bounds with lower > upper.

See also JuliaOpt/LinQuadOptInterface.jl#66

  • Needs test

collect upper and lower terms, otherwise we can end up trying to set
variable bounds with lower > upper.
@coveralls
Copy link

coveralls commented Oct 2, 2018

Coverage Status

Coverage remained the same at 0.0% when pulling 496d64e on od/fix-var-bounds into 0d57ba3 on master.

@codecov-io
Copy link

codecov-io commented Oct 2, 2018

Codecov Report

Merging #81 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #81   +/-   ##
=====================================
  Coverage       0%     0%           
=====================================
  Files           4      4           
  Lines        1602   1619   +17     
=====================================
- Misses       1602   1619   +17
Impacted Files Coverage Δ
src/MOIWrapper.jl 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d57ba3...496d64e. Read the comment docs.

@blegat
Copy link
Member

blegat commented Oct 2, 2018

What do you do in case with lower > upper ?

@odow
Copy link
Member Author

odow commented Oct 2, 2018

Good point, we should throw an error, although probably at the MOI level

@blegat
Copy link
Member

blegat commented Oct 2, 2018

In MathProgBase, we had a special treatment that was added here

@odow
Copy link
Member Author

odow commented Oct 6, 2018

Okay so we have two options:

  1. Add a check prior to solve for invalid bounds.
  2. Perform no check, and allow GLPK to return TerminationStatus = OtherError and print a nice warning saying that there are invalid bounds.

Currently, this PR does option 1. For simplicity, the current check is O(n+m) where n is the number of variables and m is the number of constraints. We could do better by recording invalid bounds at the creation and modification steps. I haven't gone that route as the bigger question is whether this check is worth it.

binaries::Vector{Int}
# Add a check prior to solve to see if there are infeasible column or row
# bounds. We perform this check manually because otherwise GLPK will fail
# with TerminationStatus OtherError due to invalid bounds. For reference see
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InvalidModel instead of OtherError, no?

@odow
Copy link
Member Author

odow commented Oct 8, 2018

Okay, now we just disable the preemptive check (maybe it should be off by default???) and then handle the status properly.

@odow
Copy link
Member Author

odow commented Oct 23, 2018

Bump on the reviews.

Copy link
Member

@blegat blegat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@odow odow merged commit 4b21e26 into master Oct 23, 2018
@odow odow deleted the od/fix-var-bounds branch October 23, 2018 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants