Remove canget #479
Remove canget #479
Conversation
Codecov Report
@@ Coverage Diff @@
## master #479 +/- ##
==========================================
- Coverage 95.44% 95.32% -0.13%
==========================================
Files 46 46
Lines 5267 4640 -627
==========================================
- Hits 5027 4423 -604
+ Misses 240 217 -23
Continue to review full report at Codecov.
|
src/Test/contconic.jl
Outdated
| if config.infeas_certificates | ||
| @test MOI.get(model, MOI.TerminationStatus()) == MOI.Success | ||
| else | ||
| @test MOI.get(model, MOI.TerminationStatus()) in [MOI.InfeasibleNoResult, MOI.InfeasibleOrUnbounded] | ||
| end | ||
| if MOI.canget(model, MOI.PrimalStatus()) | ||
| @test MOI.get(model, MOI.PrimalStatus()) == MOI.InfeasiblePoint | ||
| if config.infeas_certificates || MOI.get(model, MOI.ResultCount()) > 0 |
There was a problem hiding this comment.
The 2 "or" statements here are a bit confusing: config.infeas_certificates || MOI.get(model, MOI.ResultCount()) > 0
and MOI.get(model, MOI.PrimalStatus()) in (MOI.NoSolution, MOI.InfeasiblePoint).
Could you untangle what is expected in what cases and/or add a comment about why these different cases can happen?
Doesn't config.infeas_certificates imply that we expect MOI.get(model, MOI.ResultCount()) > 0 and PrimalStatus to be NoSolution?
Same comment applies right below.
src/Test/contconic.jl
Outdated
| @test MOI.get(model, MOI.ConstraintPrimal(), gmc) ≈ ones(n+1) atol=atol rtol=rtol | ||
|
|
||
| @test MOI.canget(model, MOI.ConstraintPrimal(), typeof(c)) | ||
| @test MOI.get(model, MOI.ConstraintPrimal(), c) ≈ n atol=atol rtol=rtol | ||
|
|
||
| # if config.duals |
There was a problem hiding this comment.
Since you're touching nearby code, could you add a comment explaining why this block is commented?
| @@ -294,13 +260,10 @@ function qcp1test(model::MOI.ModelLike, config::TestConfig) | |||
| # | |||
| return true | ||
| end | ||
| function MOI.get(model::CachingOptimizer, attr::MOI.AbstractModelAttribute) | ||
| if is_result_attribute(attr) |
There was a problem hiding this comment.
How would solver-specific result attributes work? Like GurobiIIS?
There was a problem hiding this comment.
They should implement is_result_attribute.
There was a problem hiding this comment.
We might make it an abstract subtype of AbstractModelAttribute, ... to make it clear that it should be specified. Or make is_result_attribute part of MOI
There was a problem hiding this comment.
What about using supports to decide whether to ask the optimizer or the model cache (with priority to the optimizer)? I can't easily think of a case where this would give the wrong answer, and is much simpler.
There was a problem hiding this comment.
supports is not currently defined for attributes that never appear in ListOf...AttributesSet so it does not make sense for result attibutes which are not settable
There was a problem hiding this comment.
This points to a larger design issue that needs to be resolved before tagging 0.6, but maybe this PR isn't the best place to discuss. It's definitely not ok for solvers to be required to overload functions in MOIU to get correct behavior.
There was a problem hiding this comment.
Discussion moved to #480 for more visibility
| @@ -174,52 +174,6 @@ function get!(output, model::ModelLike, attr::AnyAttribute, args...) | |||
| throw(ArgumentError("ModelLike of type $(typeof(model)) does not support accessing the attribute $attr")) | |||
| end | |||
There was a problem hiding this comment.
(Applies a few lines above.) The documentation for get needs to be updated to describe when it now returns nothing.
Here are the following changes:
cangetequal to false and not be included inListOf...AttributesSet. No, it is included in the list independently on the number of variables/constraints with this attribute set.cangetand this was used by the bridgeoptimizer and universalfallback to check whether it is the name of a bridged constraints or of a constraint of the internal model. This required 2 dictionary lookup: 1 for canget and 1 for get. If get is called with a name that does not exists an error was thrown. Now with this PR,getreturnsnothingin case the name does not exists. This allows bridges and fallback to have almost no overhead in case the name not the name of a bridged constraint as this is efficiently handled on Julia v1.0NoSolutionhas been added as a new result status which specifies that the result has no primal or dual solution, e.g. a result could have only a dual solution in which case the primal status isNoSolution.Closes #424
Closes #302