Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Indicator constraint support #709
Indicator constraint support #709
Changes from 33 commits
049e4cb
a550983
d17ba8e
3bce12e
2ca54d9
c3c2fa6
6e57763
9db213a
9c0e8b6
255c409
d741b61
232d5bf
f940ea8
6ce7a65
18e9168
8621773
c10e1b9
e7bb25b
4d889be
ab42f21
8da75e0
d15fd9a
4bc4d9e
24af034
f60c3c8
1560400
74836af
e89d734
d9565d6
3c0155e
c7f926e
9d85fee
82d3d56
01f8690
caeb4c4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check that modifying the set in
s1
does not modify the set ins1_copy
. You will need a scalar set which is mutableThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we even have one? I understand the importance of testing it does not happen, but we are calling copy on the subtest, so if error there is, it's because
copy
is not properly implemented on itThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matbesancon
LessThan{BigInt}
is mutable becauseBigInt
is mutable. You call also create a dummy mutable coefficient type or a dummy mutable set just for the test.That's what we want to test. If you remove this copy on the subset, the tests still pass while they shouldn't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blegat I played a bit with this idea. Fundamentally, this is breaking an assumption about numbers that will be everywhere in MOI, this PR doesn't change that.
BigInt
being mutable is just a constraint for their construction as far as I know, not supposed to be used.The implementation from
src/sets
:Is then false because it should check
isbits
on the setThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. We could defer the question of whether we should copy the field of EqualTo... to a separate issue.
Still you might be able to create a mutable abstract set with Float64 to test the fact we ce copy the set for IndicatorSet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blegat see issue #721, and tests added here for
IndicatorSet
copy