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 get basis status for constraints and variables #54

Merged
merged 14 commits into from
Mar 29, 2019

Conversation

EdvinAblad
Copy link
Contributor

Resolves issue jump-dev/MathOptInterface.jl#655 in MOI for Clp

@mlubin
Copy link
Member

mlubin commented Feb 22, 2019

@odow Are the tests failing because of a recent LQOI change? https://travis-ci.org/JuliaOpt/Clp.jl/jobs/496747085#L540

@EdvinAblad This looks good but we need this functionality covered by at least one test. Doing so is a bit involved:

  • Check out https://github.com/JuliaOpt/MathOptInterface.jl/blob/master/src/Test/contlinear.jl. If there's an an LP there with a unique optimal basis, then we can add on to that test, otherwise we need to create a new test problem.
  • Add a field to the TestConfig struct for checking optimal bases. It could be basis::Bool.
  • In an if config.basis block, test that the solver returns the expected basis.
  • Try to cover MOI.BASIC, MOI.NONBASIC, MOI.NONBASIC_AT_LOWER, and MOI.NONBASIC_AT_UPPER. (SUPERBASIC doesn't happen for LPs.)
  • I'll give you a break and won't ask you to add support for basis status to MOIU.MockOptimizer, but that would ideally happen if we wanted to cross our t's and dot our i's.
  • Make sure Clp passes this new test.

@odow
Copy link
Member

odow commented Feb 22, 2019

Urgh no MOI 8.2. @blegat can we just exclude the test or should it pass?

@blegat
Copy link
Member

blegat commented Feb 22, 2019

Solvers that use a CachingOptimizer in their tests don't have to excluded it, it just displays a warning. However, if the solver does not use a CachingOptimizer, it should exclude it.
So for Clp, it should be excluded

@EdvinAblad
Copy link
Contributor Author

EdvinAblad commented Feb 23, 2019

@mlubin I have a working (unpublished) test but two questions occurred:

  1. Should I also implement the get for the "Interval Bridge" or is that a separate issue?
  2. To me there is a redundancy in the nonbasic status. i.e., NON_BASIC_AT_LOWER, and NON_BASIC_AT_UPPER both imply NON_BASIC. E,g, if a variable is non_basis should I test NON_BASIC or the specific NON_BASIC_AT_LOWER. Should NON_BASIC be renamed to NON_BASIC_FIXED?

@blegat
Copy link
Member

blegat commented Feb 23, 2019

Should I also implement the get for the "Interval Bridge" or is that a separate issue?

For the interval bridge, it is an MOI issue: https://github.com/JuliaOpt/MathOptInterface.jl/blob/master/src/Bridges/intervalbridge.jl

@blegat
Copy link
Member

blegat commented Mar 7, 2019

I suppose we need to bump MOI to v0.8.3 in the REQUIRE file

@EdvinAblad
Copy link
Contributor Author

I suppose we need to bump MOI to v0.8.3 in the REQUIRE file

Clp is indirectly dependent on MOI through the LQOI interface.
Should we temporarily add MOI v0.8.3 as a requirement (until there exist a LQOI version with this requirement)?

(It should "only" be the tests of Clp that require MOI v0.8.3, if this makes a difference)

@blegat
Copy link
Member

blegat commented Mar 7, 2019

If it's only the tests, it's less of a big deal but a user would still expects

] add Clp
] test Clp

to work so adding a MOI lower bound wouldn't hurt. LQOI doesn't require MOI v0.8.3 at the moment but if it does later we may remove the MOI lower bound here.

@EdvinAblad
Copy link
Contributor Author

Is there something preventing this from being merged into master?

@mlubin
Copy link
Member

mlubin commented Mar 23, 2019 via email

@EdvinAblad
Copy link
Contributor Author

Yes, I'm on vacation :). I have style comments that I may or may not be able to send out before next week.

On Sat, Mar 23, 2019, 14:27 EdvinAblad @.***> wrote: Is there something preventing this from being merged into master? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#54 (comment)>, or mute the thread https://github.com/notifications/unsubscribe-auth/ABp0M7RKS-iZIg-x_dqyV3qGA01kdcvCks5vZmP8gaJpZM4bIcRU .

Ok, there is really no rush, enjoy the vacation =)

@mlubin mlubin merged commit 81c2c9a into jump-dev:master Mar 29, 2019
@mlubin
Copy link
Member

mlubin commented Mar 29, 2019

Thanks!

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.

4 participants