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.

Sign up for GitHub

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

Merged
merged 35 commits into from
Apr 30, 2019
Merged

Indicator constraint support #709

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
049e4cb
first draft indicator cons
matbesancon Apr 11, 2019
a550983
fixed docstring
matbesancon Apr 11, 2019
d17ba8e
fixed LessThan
matbesancon Apr 11, 2019
3bce12e
fix import
matbesancon Apr 11, 2019
2ca54d9
last SmallerThan
matbesancon Apr 12, 2019
c3c2fa6
copy correct impl
matbesancon Apr 12, 2019
6e57763
generic vec
matbesancon Apr 12, 2019
9db213a
Revert "generic vec"
matbesancon Apr 12, 2019
9c0e8b6
changed dim and doc
matbesancon Apr 12, 2019
255c409
ActiveCond type parameter
matbesancon Apr 13, 2019
d741b61
enums + constraint
matbesancon Apr 14, 2019
232d5bf
capital enum values
matbesancon Apr 15, 2019
f940ea8
relax type constraint on S
matbesancon Apr 16, 2019
6ce7a65
latex escaping, naming
matbesancon Apr 17, 2019
18e9168
explicit set name
matbesancon Apr 18, 2019
8621773
test 1 indicator
matbesancon Apr 18, 2019
c10e1b9
penalized version
matbesancon Apr 18, 2019
e7bb25b
add reference, fixed doc, tests
matbesancon Apr 19, 2019
4d889be
Merge branch 'master' into indicator-cons
matbesancon Apr 19, 2019
ab42f21
fix test formatting and errors
matbesancon Apr 23, 2019
8da75e0
fix conflict
matbesancon Apr 23, 2019
d15fd9a
test in main, example usage
matbesancon Apr 23, 2019
4bc4d9e
split tests
matbesancon Apr 23, 2019
24af034
fix test 2
matbesancon Apr 23, 2019
f60c3c8
added test
matbesancon Apr 23, 2019
1560400
remove unsupported tests, names
matbesancon Apr 24, 2019
74836af
test passing
matbesancon Apr 24, 2019
e89d734
fix support
matbesancon Apr 24, 2019
d9565d6
fix style
matbesancon Apr 24, 2019
3c0155e
copy implementation and test
matbesancon Apr 24, 2019
c7f926e
fix comments, added test for ACTIVE_ON_ZERO
matbesancon Apr 25, 2019
9d85fee
test support indicator
matbesancon Apr 25, 2019
82d3d56
explicit comment
matbesancon Apr 25, 2019
01f8690
fix conflict
matbesancon Apr 28, 2019
caeb4c4
add mutable test
matbesancon Apr 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/sets.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,10 @@ end

dimension(::IndicatorSet) = 2

function Base.copy(set::IndicatorSet{A,S}) where {A,S}
return IndicatorSet{A}(copy(set.set))
end

# isbits types, nothing to copy
function Base.copy(set::Union{Reals, Zeros, Nonnegatives, Nonpositives,
GreaterThan, LessThan, EqualTo, Interval,
Expand All @@ -492,7 +496,7 @@ function Base.copy(set::Union{Reals, Zeros, Nonnegatives, Nonpositives,
PositiveSemidefiniteConeSquare,
LogDetConeTriangle, LogDetConeSquare,
RootDetConeTriangle, RootDetConeSquare,
Integer, ZeroOne, Semicontinuous, Semiinteger, IndicatorSet})
Integer, ZeroOne, Semicontinuous, Semiinteger})
return set
end
Base.copy(set::S) where {S <: Union{SOS1, SOS2}} = S(copy(set.weights))
10 changes: 10 additions & 0 deletions test/sets.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,15 @@
s_copy.weights[1] = 2.0
@test s.weights[1] == 1.0
end
@testset "IndicatorSet" begin
s1 = MOI.IndicatorSet{MOI.ACTIVATE_ON_ONE}(MOI.LessThan(4.0))
s2 = MOI.IndicatorSet{MOI.ACTIVATE_ON_ZERO}(MOI.GreaterThan(4.0))
s1_copy = copy(s1)
s2_copy = copy(s2)
@test s1_copy isa MOI.IndicatorSet{MOI.ACTIVATE_ON_ONE}
@test s1 == s1_copy
Copy link
Member

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 in s1_copy. You will need a scalar set which is mutable

Copy link
Contributor Author

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 it

Copy link
Member

@blegat blegat Apr 27, 2019 •
edited
Loading

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 because BigInt is mutable. You call also create a dummy mutable coefficient type or a dummy mutable set just for the test.

we are calling copy on the subtest

That's what we want to test. If you remove this copy on the subset, the tests still pass while they shouldn't

Copy link
Contributor Author

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:

# isbits types, nothing to copy
function Base.copy(set::Union{Reals, Zeros, Nonnegatives, Nonpositives,
                              GreaterThan, LessThan, EqualTo, Interval,
                              SecondOrderCone, RotatedSecondOrderCone,
                              GeometricMeanCone, ExponentialCone,
                              DualExponentialCone, PowerCone, DualPowerCone,
                              PositiveSemidefiniteConeTriangle,
                              PositiveSemidefiniteConeSquare,
                              LogDetConeTriangle, LogDetConeSquare,
                              RootDetConeTriangle, RootDetConeSquare,
                              Integer, ZeroOne, Semicontinuous, Semiinteger})
    return set
end

Is then false because it should check isbits on the set

Copy link
Member

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

Copy link
Contributor Author

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

@test s2_copy isa MOI.IndicatorSet{MOI.ACTIVATE_ON_ZERO}
@test s2 == s2_copy
end
end
end