SingleVariable constraint have same index as the variable#732
SingleVariable constraint have same index as the variable#732
Conversation
Codecov Report
@@ Coverage Diff @@
## master #732 +/- ##
==========================================
- Coverage 94.24% 94.02% -0.22%
==========================================
Files 55 55
Lines 5852 6124 +272
==========================================
+ Hits 5515 5758 +243
- Misses 337 366 +29
Continue to review full report at Codecov.
|
blegat
force-pushed
the
bl/single_variable_index
branch
from
b3961d6 to
a784423
Compare
| function MOI.get(b::AbstractBridgeOptimizer, | ||
| attr::MOI.ListOfConstraintIndices{F, S}) where {F, S} | ||
| # List of indices of all constraints, even those bridged | ||
| function get_all_even_bridged( |
There was a problem hiding this comment.
Why does _remove_bridged have an underscore and this function doesn't?
There was a problem hiding this comment.
I don't want to user to call _remove_bridged as it could corrupt the integrity of the object.
get_all_including_bridged provides a well defined information which could be useful for inspecting and debugging.
| a = MOI.add_variable(model) | ||
| b = MOI.add_variable(model) | ||
| vc1 = MOI.add_constraint(model, MOI.SingleVariable(a), MOI.EqualTo(0.5)) | ||
| @test vc1.value == a.value |
There was a problem hiding this comment.
I don't think we should litter the longer tests with this check after every SingleVariable() constraint. This distracts from the focus of the test which is to check that the solution is correct. Other tests check the value of the indices.
There was a problem hiding this comment.
Which other tests ? Some solvers do not call all the tests and many implementation would get singlevariable indices equal to the variables indices just by chances so I wanted to optimize the chance to catch it by checking it in as many corner cases as possible.
There was a problem hiding this comment.
It's good to catch it in corner cases, but this can be done by creating standalone unit tests. If solvers aren't running the right tests, that's a separate issue that should be addressed.
Doing the check here interferes with the readability of the tests. Tests should have a clear purpose and limited scope so that it's easy to understand what they're doing, how to modify them, and if they're correct. This is a particularly bad experience for someone who wants to add a new test and tries to copy the existing style. Are they responsible for adding these checks for every SingleVariable constraint in the test?
There was a problem hiding this comment.
It's good to catch it in corner cases, but this can be done by creating standalone unit tests
If solvers aren't running the right tests, that's a separate issue that should be addressed
The right tests might be using constraints that are bridged so even if the optimizer runs these tests, it will work just because the bridgeoptimizer is correct. If the solver only supports SingleVariable for a specific constraint and has a bug when this constraint is added in some order, it needs to be tested on that test in that order. We cannot create every corner case in unit tests.
This is a particularly bad experience for someone who wants to add a new test and tries to copy the existing style.
I understand this concern but I feel like having a good testing coverage of indices of SingleVariable constraints is more important.
In the long term, we might replace these tests with reading from a problem described in text like in unit tests and the check could be done automatically in a common function that calls the reading and then do common checks. The coverage for some attributes is currently not ideal, e.g. all the tests pass although we have this bug #693 because this bug only happens in corner cases and we don't check MOI.ListOfVariableIndices very often.
Are they responsible for adding these checks for every SingleVariable constraint in the test?
I think we should but it already has a decent coverage with existing tests so it might not be critical for new tests if they do not test new scalar sets.
There was a problem hiding this comment.
In the long term, we might replace these tests with reading from a problem described in text like in unit tests and the check could be done automatically in a common function that calls the reading and then do common checks.
Sounds like a good idea.
I'll withdraw my objection if you add an explanatory comment on the first appearance in every file of @test vc1.value == a.value (or the equivalent) in a large solve-and-check-solution test (like rotatedsoc1test and int1test). I think this amount of documentation is proportional to the violation of readability that these tests incur.
3 participants
Closes #570