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

[FileFormats] read double-sided variable bounds separately #2548

Merged
merged 3 commits into from
Oct 7, 2024

Conversation

odow
Copy link
Member

@odow odow commented Oct 7, 2024

Closes #2547

Comment on lines 1289 to 1295
set = bounds_to_set(data.col_lower[i], data.col_upper[i])
if set !== nothing
if set isa MOI.Interval
MOI.add_constraint(model, x, MOI.GreaterThan(set.lower::Float64))
MOI.add_constraint(model, x, MOI.LessThan(set.upper::Float64))
elseif set !== nothing
MOI.add_constraint(model, x, set)
end

Choose a reason for hiding this comment

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

Sorry if this is too nit-picky, but: Could creating the "helper" Interval set be an overhead for large files?

Something like this would be possible here (but makes it more convoluted):

if isfinite(data.col_lower[i])
    MOI.add_constraint(model, x, MOI.GreaterThan(data.col_lower[i]::Float64))
end

if isfinite(data.col_upper[i])
    MOI.add_constraint(model, x, MOI.LessThan(data.col_upper[i]::Float64))
end

This would also handle two edge cases differently:

  1. If data.col_lower[i] > data.col_upper[i], it adds the bounds keeping the model infeasible (bounds_to_set would currently remove the bounds and create a free variable)
  2. If data.col_lower[i] == data.col_upper[i], it sticks to adding bounds, and does not convert those into a single equality

(1.) is maybe not really important, but (2.) could lead to similar problems as the initial version:

  • I create a variable that somewhere along the way gets equal bounds, then
  • write that to a file, read it back in, and
  • the bounds are gone, has_*_bound will return false, and I can't change the bounds anymore.

Further, this also impacts how fix(x, value; force=false) behaves. With bounds it errors, with the EqualTo it overwrites.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could creating the "helper" Interval set be an overhead for large files?

Nope. It's a struct that is on the stack.

Something like this would be possible here

bounds_to_set already does this 😄 it will return an Interval only if both sides are finite.

the bounds are gone, has_*_bound will return false, and I can't change the bounds anymore.

You'd need to unfix(x); set_lower_bound(x, lb).

The issue is that JuMP doesn't offer a nice way to interact with interval bounds.

I agree that lower/upper/fixed bounds are more convoluted than a plain lower/upper bound. You'll need to write some JuMP code to deal with the different situations.

test/FileFormats/MPS/MPS.jl Outdated Show resolved Hide resolved
@odow odow merged commit f82c7e9 into master Oct 7, 2024
16 checks passed
@odow odow deleted the od/bound-interval branch October 7, 2024 20:25
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.

Inconsistent "interval" constraints when reading models from file
2 participants