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

Tidy src/variable.jl #583

Merged
merged 2 commits into from
Jan 28, 2024
Merged

Tidy src/variable.jl #583

merged 2 commits into from
Jan 28, 2024

Conversation

odow
Copy link
Member

@odow odow commented Jan 23, 2024

@ericphanson thoughts on :Bin vs BinVar? I assume your long-term plan was to deprecate the symbols? They're pretty brittle and the docs don't mention the Symbol approach.

@ericphanson
Copy link
Collaborator

Yeah, that was the plan. After using it intermittently though I don’t really like the *Var suffixes; they are hard to remember and look a bit weird. I’m definitely open to alternative approaches!

@odow
Copy link
Member Author

odow commented Jan 23, 2024

What about MOI.ZeroOne and MOI.Integer, or the Base types Int and Bool?

@ericphanson
Copy link
Collaborator

Either of those seems better! The MOI ones might help folks connect the dots with MOI more, while the Base ones might look less scary. Both could lead to the issue of folks trying other objects that aren’t supported, but we could give good errors.

I see JuMP supports Int, Bin, and binary=true, so that could also make sense here.

@ericphanson
Copy link
Collaborator

Btw now that JuMP supports generic number types, it seems ok to lower to jump rather than MOI- maybe that could allow better interop, like mixing Convex and JuMP variables in a model. I bring this up here since I suspect Bin is from JuMP, not MOI.

@odow
Copy link
Member Author

odow commented Jan 23, 2024

I think we want to keep lowering to MOI instead of JuMP. We have some ideas for DCP in JuMP. One is to lower JuMP's nonlinear expressions to Convex (requires jump-dev/MathOptInterface.jl#2402).

The other is to add DCP support to JuMP directly. Working through this code base is giving me some ideas. But this is a long-term thing. Nothing in the short-term.

test/test_utilities.jl Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 23, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (0e42f97) 97.76% compared to head (56e8132) 97.94%.

Files Patch % Lines
src/variable.jl 93.75% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #583      +/-   ##
==========================================
+ Coverage   97.76%   97.94%   +0.18%     
==========================================
  Files          91       91              
  Lines        5060     5069       +9     
==========================================
+ Hits         4947     4965      +18     
+ Misses        113      104       -9     

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

@blegat
Copy link
Member

blegat commented Jan 23, 2024

Btw now that JuMP supports generic number types, it seems ok to lower to jump rather than MOI- maybe that could allow better interop, like mixing Convex and JuMP variables in a model

You can already mix any constraints supported by JuMP with DCP constraints when using Convex.Optimizer, what would using Convex at the JuMP level achieve ? Mixing the Convex expressions and the JuMP expressions is tricky, it was the approach of ParameterJuMP but we're now going the other direction with ParametricOptInterface. I feel staying at the MOI level where you only need to care about the MOI API and not arbitrary interaction with the user is easier :)

@odow
Copy link
Member Author

odow commented Jan 24, 2024

see JuMP supports Int, Bin, and binary=true, so that could also make sense here.

@odow
Copy link
Member Author

odow commented Jan 24, 2024

see JuMP supports Int, Bin, and binary=true, so that could also make sense here.

Ah, but it is the symbol :Int in the macro and not the Base.Int type. And same with :Bin, because Bin does not exist as a type (or object) in Base or JuMP...

Variable(2, 3; binary = true) is okay. So is Variable(Bool) and Variable(Int). You can't mix Bool and Int, and you can do Semidefinite(2, Int) or Semidefinite(2, Bool) if you reeeeally wanted? (I don't know if we really have a MISDP solver.)

Perhaps this is a separate PR through.

@odow odow merged commit e5b218f into master Jan 28, 2024
10 of 11 checks passed
@odow odow deleted the od/variable branch January 28, 2024 23:01
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.

3 participants