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

Add start_value, lower_bound, and upper_bound support for GenericAffExpr #3551

Merged
merged 7 commits into from Oct 31, 2023

Conversation

odow
Copy link
Member

@odow odow commented Oct 29, 2023

Closes #3550

This PR is up for debate, but our problem is that some usages of @variable return expressions of variables, and then common operations like lower_bound or set_start_value do not work.

In most common cases we can make this work, so we probably should. This would improve the user-experience, particularly for those using Complex number support.

src/aff_expr.jl Outdated Show resolved Hide resolved
src/aff_expr.jl Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (35e7713) 98.26% compared to head (4e256ef) 98.19%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3551      +/-   ##
==========================================
- Coverage   98.26%   98.19%   -0.07%     
==========================================
  Files          37       37              
  Lines        5580     5599      +19     
==========================================
+ Hits         5483     5498      +15     
- Misses         97      101       +4     
Files Coverage Δ
src/aff_expr.jl 97.54% <100.00%> (+0.19%) ⬆️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/aff_expr.jl Outdated Show resolved Hide resolved
src/aff_expr.jl Outdated Show resolved Hide resolved
@odow
Copy link
Member Author

odow commented Oct 29, 2023

Hmm. There are actually a bunch of edge cases once you dig in:

    model = Model()
    @variable(model, x)
    y = (1 - 2im) * x
    @test !has_lower_bound(y)
    set_lower_bound(y, 1 + 2im)

What is lower_bound(x)? Maybe we should only support expressions that have +/-1 or +/-im as coefficients.

@mlubin
Copy link
Member

mlubin commented Oct 29, 2023

My sense is that this might cause more confusion than it solves. Next we'll have a request to support lower_bound and upper_bound for nonlinear expressions, which is not trivial to implement.

@blegat
Copy link
Member

blegat commented Oct 29, 2023

Setting bounds should only be allowed with one term and real coefficient. If complex, the user can be explicit by doing set_lower_bound(imag(aff), ...)

@odow
Copy link
Member Author

odow commented Oct 29, 2023

My sense is that this might cause more confusion than it solves.

Yeah. But it's currently causing confusion because some outputs from @variable are actually AffExpr, and even something like real(x) is not a variable but an AffExpr.

Setting bounds should only be allowed with one term and real coefficient

This is reasonable.

@mlubin
Copy link
Member

mlubin commented Oct 29, 2023

Setting bounds should only be allowed with one term and real coefficient

That's a fair compromise.

@odow
Copy link
Member Author

odow commented Oct 29, 2023

How about this now

@odow odow requested a review from blegat October 30, 2023 20:12
src/aff_expr.jl Outdated Show resolved Hide resolved
test/test_expr.jl Outdated Show resolved Hide resolved
src/aff_expr.jl Outdated Show resolved Hide resolved
@odow
Copy link
Member Author

odow commented Oct 30, 2023

Removed the method. How's the now

@odow odow merged commit 908275f into master Oct 31, 2023
11 of 12 checks passed
@odow odow deleted the od/aff-expr-start-bound branch October 31, 2023 19:16
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.

set_start_value for complex variables
3 participants