RFC: new modification syntax #388

odow · 2018-06-14T03:13:52Z

Overview

This pull request changes the modification syntax in MOI.

The biggest change is to refactor modifyconstraint!(m, c::ConstraintIndex{F,S}, set::S) to set!(m, ::ConstraintSet, c::ConstraintIndex{F,S}, set::S). (A similar refactor is done with ConstraintFunction.) This plays much nicer with the get/set! paradigm.

In addition, modifyconstraint! methods that implement AbstractFunctionModification is refactored to modify! and transformconstraint! is refactored into transform!.

This PR also adds tests and documentation.

Changes

See #360 for discussion. Proposed changes:

set!(m::ModelLike, ::ConstraintFunction, c::ConstraintIndex{F,S}, new_function::F)
set!(m::ModelLike, ::ConstraintSet, c::ConstraintIndex{F,S}, new_set::S)
~~@blegat, how do I make the change for ConstraintSet in model.jl? There is quite a few levels of in-direction and the macro and I don't know what to touch.~~
modify!(m::ModelLike, c::ConstraintIndex{F,S}, change::AbstractFunctionModification)
(Note: this is slightly different to that proposed in Proposal: change API for modifications #360.)
modify!(m::ModelLike, ::ObjectiveFunction, change::AbstractFunctionModification)
transformconstraint! -> transform!

Tests

Blockers / future PR

cantransform still takes the instance of the constraint index of an argument rather than the type. This is needed because there is a default fallback and candelete also takes the instance rather than the type (see candelete is inconsistent with canget #218).
transform! is not tested as it is not implemented in MockOptimizer.
transform! only applies to constraint sets. Should it apply to constraint functions for consistency? I'm not sure if it makes sense to change the function to a different type of function without deleting the constraint though...

These points can probably be addressed by a future PR that changes candelete and cantransform at the same time and adds tests. This PR is already quite large.

`set!(m, ConstraintSet(), c, set)`. This patch currently errors as it needs to be implemented for src/Utilities/model.jl, but I'm not sure where to start.

blegat · 2018-06-14T06:47:28Z

Do we now disallow a set or function being passed to modify! ?

odow · 2018-06-14T07:00:59Z

Yes. modify will be reserved for AbstractFunctionModifications.

blegat · 2018-06-14T07:04:50Z

src/Utilities/mockoptimizer.jl

-function MOI.modifyconstraint!(mock::MockOptimizer, c::CI{F,S}, set::S) where {F<:MOI.AbstractFunction, S<:MOI.AbstractSet}
-    MOI.modifyconstraint!(mock.inner_model, xor_index(c), set)
+MOI.canset(mock::MockOptimizer, ::MOI.ConstraintSet, c::CI{F,S}, set::Type{S}) where {F<:MOI.AbstractFunction, S<:MOI.AbstractSet} = true
+function MOI.set!(mock::MockOptimizer, ::MOI.ConstraintSet, c::CI{F,S}, set::S) where {F<:MOI.AbstractFunction, S<:MOI.AbstractSet}


This is already handled by line 94

blegat · 2018-06-14T07:13:15Z

src/constraints.jl

+set!(model, ConstraintSet(), c, NonPositives)    # Error
+```
+"""
+function set!(model::ModelLike, cs::ConstraintSet, c::ConstraintIndex{F,S1}, s::S2) where F where  S1 where S2


The issue of having a specific method like this is that it prevents models to handle it with a more general method as this would lead to ambiguity (this affects mockoptimizer, bridges, ...)
We could instead call something like seterror in line 234.
The default seterror method would give the ArgumentError currently at line 234 and we would add this method as a seterror method.

blegat · 2018-06-14T07:15:17Z

Why don't we use a different signature for canset than the one used for all other attributes ?

@blegat

Updates from @blegat

codecov-io · 2018-06-14T23:31:43Z

Codecov Report

Merging #388 into master will increase coverage by 0.15%.
The diff coverage is 93.22%.

@@            Coverage Diff             @@
##           master     #388      +/-   ##
==========================================
+ Coverage   96.35%   96.51%   +0.15%     
==========================================
  Files          39       40       +1     
  Lines        4913     5676     +763     
==========================================
+ Hits         4734     5478     +744     
- Misses        179      198      +19

Impacted Files	Coverage Δ
src/MathOptInterface.jl	`100% <ø> (ø)`	⬆️
src/Test/UnitTests/unit_tests.jl	`100% <ø> (ø)`	⬆️
src/modifications.jl	`0% <0%> (ø)`
src/attributes.jl	`84.61% <0%> (-9.01%)`	⬇️
src/Utilities/mockoptimizer.jl	`99.5% <100%> (+2.01%)`	⬆️
src/Test/UnitTests/modifications.jl	`100% <100%> (ø)`
src/Bridges/rsocbridge.jl	`100% <100%> (ø)`	⬆️
src/Bridges/detbridge.jl	`98.82% <100%> (+0.05%)`	⬆️
src/Test/contlinear.jl	`100% <100%> (ø)`	⬆️
src/Utilities/model.jl	`98.73% <100%> (+0.02%)`	⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef32b86...622f009. Read the comment docs.

canmodify. These functions now only apply to AbstractFunctionModification changes.

problems with canmodify in the bridge code.

odow · 2018-06-15T02:39:05Z

src/Bridges/bridgeoptimizer.jl

+    if isbridged(b, C)
+       MOI.canmodify(b.bridged, C, Chg)
+       # TODO(@blegat): how should the following be implemented?
+       # && MOI.canmodify(b, MOIB.bridge(b, ci), Chg)


Okay, @blegat new question. Is this necessary and in what use-case? It makes tests like
https://github.com/JuliaOpt/MathOptInterface.jl/pull/388/files#diff-5d9f4af23e8ec976db542b6b9af239c4R135
break, but everything else passes. There are also things like
https://github.com/JuliaOpt/MathOptInterface.jl/pull/388/files#diff-91d11cfb910f6ae0036ae89cac114d3eR144
Are they necessary?

Yes, it checks that the bridge supports the modification. Currently, only the interval bridge supports modification

I missed the second part to the question. If they are necessary, how do we implement this using only type information...

You need bridgetype :)

I have pushed a fix

odow · 2018-06-15T02:39:36Z

src/Bridges/bridgeoptimizer.jl

+    if isbridged(b, C)
+       MOI.canset(b.bridged, MOI.ConstraintSet(), C)
+        # TODO(@blegat) is this necessary? How do I do it without types
+        # && MOI.canset(b, MOIB.bridge(b, ci), change)


Ditto here.

odow · 2018-06-15T02:39:46Z

src/Bridges/bridgeoptimizer.jl

+    if isbridged(b, C)
+       MOI.canset(b.bridged, MOI.ConstraintFunction(), C)
+        # TODO(@blegat) is this necessary? How do I do it without types
+        # && MOI.canset(b, MOIB.bridge(b, ci), change)


and one commented out as MockOptimizer fails getting the constraint function.

odow · 2018-06-16T03:47:36Z

src/Test/UnitTests/modifications.jl

+    )
+    @test MOI.canget(model, MOI.ConstraintFunction(), typeof(c))
+    foo = MOI.get(model, MOI.ConstraintFunction(), c)
+    @test_broken foo == MOI.ScalarAffineFunction([MOI.ScalarAffineTerm(2.0, x)], 0.0)


@blegat another mystery. The VariableIndex in foo is VariableIndex(12345679) instead of VariableIndex(1), but I couldn't work out where the missing xor is.

Fixed by 1ad6f61

associated bug.

tested as transform! is not implemented in MockOptimizer

blegat · 2018-06-17T08:55:46Z

src/Bridges/bridgeoptimizer.jl

+    end
+end
+function MOI.set!(b::AbstractBridgeOptimizer, ::MOI.ConstraintFunction, constraint_index::CI, func)
+    # Note: we also under-type this function to avoid ambiguity


What do you mean by this comment ? Why don't you just define set!(..., attr::Union{ConstraintFunction, ConstraintSet}, ...) ?

I had a hard time getting rid of method ambiguities since there are quite a few abstract subtypes of ModelLike floating around. With the Union there is an ambiguity with set!(::ModelLike, ::ConstraintFunction, ...). as the first argument is more specific but the second is less specific. I've typed this now and it seems to all work.

mlubin · 2018-06-17T13:37:22Z

docs/src/apimanual.md

+set!(m, ConstraintSet(), c, GreaterThan(2.0))  # errors
+```
+If our constraint is an affine inequality, then this corresponds to modifying
+the right hand-side of a constraint in linear programming.


Nit: right-hand side

mlubin · 2018-06-17T13:55:18Z

docs/src/apimanual.md

+Also note that `transform!` cannot be called with a set of the same type;
+[`set!`](@ref) should be used instead.
+
+### Modify the function of a constraint


The use of "modify" here could be a bit confusing given that this is discussing replacing the function rather than using an AbstractFunctionModification. It maybe be worth restructuring a bit to clarify, e.g., by grouping all the AbstractFunctionModifications together somehow.

mlubin · 2018-06-17T13:57:24Z

docs/src/apimanual.md

+
+### MultirowChange
+
+Finally, the last modification supports by MathOptInterface is the ability to


Nit: supported

mlubin · 2018-06-17T13:58:34Z

docs/src/apimanual.md

+### MultirowChange
+
+Finally, the last modification supports by MathOptInterface is the ability to
+modify the affine coefficient of a single variable in a VectorAffine or


affine coefficients

mlubin · 2018-06-17T20:29:21Z

docs/src/apimanual.md

+### MultirowChange
+
+Finally, the last modification supports by MathOptInterface is the ability to
+modify the affine coefficient of a single variable in a VectorAffine or


It's a bit clearer to say more explicitly "in a VectorAffineFunction or a VectorQuadraticFunction".

mlubin · 2018-06-17T20:48:20Z

src/Utilities/universalfallback.jl

+function MOI.canset(uf::UniversalFallback, ::MOI.ConstraintSet, ::Type{C}) where C <: CI
+    MOI.canset(uf.model, MOI.ConstraintSet(), C)
+end
+function MOI.set!(uf::UniversalFallback, ::MOI.ConstraintSet, ci::CI{F,S}, set::S) where F where S


I think where {F,S} is more idiomatic than where F where S, but correct me if I'm wrong.

mlubin · 2018-06-17T20:56:50Z

src/constraints.jl

-function canmodifyconstraint end
-canmodifyconstraint(model::ModelLike, c::ConstraintIndex, change) = false
+canset(model::ModelLike, ::ConstraintSet, type_of_constraint_set) = false
+# Note: the above method is deliberately under-typed to avoid method ambiguities


Add an example of which method ambiguity is being avoided, in case we refactor later and it's no longer necessary.

mlubin · 2018-06-17T21:00:08Z

src/constraints.jl

-modifyconstraint!(model, c, Interval(0, 5))
-modifyconstraint!(model, c, NonPositives) # Error
+set!(model, ConstraintSet(), c, Interval(0, 5))
+set!(model, ConstraintSet(), c, NonPositives)    # Error


Nonpositives is the type. Maybe use a scalar set instead so it's a bit more clear, e.g., LessThan(3).

mlubin · 2018-06-17T21:03:39Z

src/constraints.jl

-cantransformconstraint(model, c, GreaterThan(0.0)) # true
-cantransformconstraint(model, c, ZeroOne())        # false
+cantransform(model, c, GreaterThan(0.0)) # true
+cantransform(model, c, ZeroOne())        # false


This is false for all current solvers, but MOI allows you to implement a solver where this could work. Maybe clarify that this is expected behavior but not a hard rule.

mlubin · 2018-06-17T21:06:11Z

src/modifications.jl

+
+## Objective Function
+
+canmodify(model::ModelLike, ::ObjectiveFunction, ::Type{M})::Bool where M<:AbstractFunctionModification


Missing code formatting?

blegat · 2018-06-18T07:37:47Z

src/Bridges/bridgeoptimizer.jl

+    end
+end
+
+function MOI.canset(b::AbstractBridgeOptimizer, attr::MOI.ConstraintSet, ::Type{CI{F, S}}) where {F, S}


This can be merged with the ConstraintFunction or not finally ? I thought you said in your comment that it was updated

It is ambiguous with calls like
https://github.com/JuliaOpt/MathOptInterface.jl/blob/3ffd5ab6851260d84aa1d85ff4f2cda11b3ad870/src/constraints.jl#L52

See #388 (comment)

blegat · 2018-06-18T07:40:31Z

src/Utilities/universalfallback.jl

+MOI.canmodify(uf::UniversalFallback, ci::Type{<:CI}, change) = MOI.canmodify(uf.model, ci, change)
+MOI.modify!(uf::UniversalFallback, ci::CI, change) = MOI.modify!(uf.model, ci, change)
+
+function MOI.canset(uf::UniversalFallback, ::MOI.ConstraintSet, ::Type{C}) where C <: CI


Again, do we really need 4 functions instead of two ?

blegat · 2018-06-18T07:44:50Z

src/constraints.jl

 ```
+"""
+function set!(model::ModelLike, ::ConstraintSet, constraint_index, set)


We shouldn't define set! and canset methods with so specific types.
We should do set!(model::ModelLike, attr::AbstractConstraintAttribute, ci::CI, value) = default_set!(model, attr, ci, value) and then in the methods of default_set! we can do any typing we like without risk of ambiguity

odow · 2018-06-18T23:59:58Z

I have moved canset and set! out of the constraints.jl file and added them to the original docstring.
I've also implemented @blegat's idea of a hidden_set! function which we can overload to throw errors without causing ambiguity.
I have also removed a few of the separate canset(m, ::ConstraintSet and canset(m, ::ConstraintFunction methods in favor of ::Union{ConstraintSet,ConstraintFunction} methods.

There are still a few unresolved tick-boxes in the description of this PR, but they can be probably be addressed in future PRs.

blegat · 2018-06-19T07:54:45Z

src/attributes.jl

@@ -527,13 +578,35 @@ It is guaranteed to be equivalent but not necessarily identical to the function
 """
 struct ConstraintFunction <: AbstractConstraintAttribute end

+function hidden_set!(model::ModelLike, ::ConstraintFunction, constraint_index::ConstraintIndex, func::AbstractFunction)
+    if func_type(constraint_index) != typeof(func)


This can be done with multiple dispatch,
hidden_set!(..., ::ConstraintFunction, ::CI{F}, ::F) -> same constraint set
hidden_set!(..., ::ConstraintFunction, ::CI, ::AbstractFunction) -> different constraint set

Oh yeah good point. This is a hold-over from before

blegat · 2018-06-19T07:55:34Z

src/attributes.jl

 """
    ConstraintSet()

 Return the `AbstractSet` object used to define the constraint.
 """
 struct ConstraintSet <: AbstractConstraintAttribute end

+function hidden_set!(model::ModelLike, ::ConstraintSet, constraint_index::ConstraintIndex, set::AbstractSet)
+    if set_type(constraint_index) != typeof(set)


Same, here, it is cleaner to do that using multiple dispatch

blegat · 2018-06-19T07:59:18Z

src/Utilities/cachingoptimizer.jl

+end
+
+# This function avoids duplicating code, but allows us to strongly type the
+# arguments of the set! functions below.


I didn't get that immediately, mentioning the 4th argument explicitly in the comment would be clearer

blegat

Looks good, the new API is a lot better :)

mlubin

LGTM, just a couple additional minor comments.

mlubin · 2018-06-19T15:19:10Z

docs/src/apimanual.md

+)
+```
+
+### In-place Modifications


Nit: Inconsistent capitalization of words in section titles. (Also "Problem Modification" at the top).

mlubin · 2018-06-19T15:22:47Z

test/model.jl

@@ -123,3 +123,28 @@ end
    @test MOI.dimension(s) == 1

 end
+
+struct Func388 <: MOI.AbstractScalarFunction


Nit: use a more descriptive name or add a link to the PR if we need to reference the discussion. 388 isn't too meaningful on its own.

Begin refactor from modifyconstraint(m, c, set) to

3f7be94

`set!(m, ConstraintSet(), c, set)`. This patch currently errors as it needs to be implemented for src/Utilities/model.jl, but I'm not sure where to start.

blegat reviewed Jun 14, 2018

View reviewed changes

blegat mentioned this pull request Jun 14, 2018

Update Model #389

Merged

blegat and others added 3 commits June 14, 2018 19:53

Update Model (#389)

98f82af

Updates from @blegat

Resolve ambiguity errors relating to canset and set! ConstraintSet.

49d118b

Fixes to get tests passing

82dc333

odow added 4 commits June 15, 2018 12:08

Refactor modfiyconstraint!(m,c,f) to set!(m,ConstraintFunction(),c,f)

bf5bcc4

Refactor modifyconstraint! to modify! and canmodifycontraint to

8822416

canmodify. These functions now only apply to AbstractFunctionModification changes.

Fix definition of canmodify. This patch does not pass tests due to

35b46e2

problems with canmodify in the bridge code.

Add broken tests

136b03e

odow commented Jun 15, 2018

View reviewed changes

odow added 6 commits June 15, 2018 15:09

Add unit tests for setting ConstraintSet

0c25804

Refactor modifyobjective! to modify!(m, ObjectiveFunction, change).

db7b687

Add more tests for modifications

8097841

Begin documentation of problem modificatino

bf46749

Add more modification tests

32c0b5a

Add test for modifying constraint function. There is a broken test

b8fb3bf

and one commented out as MockOptimizer fails getting the constraint function.

odow commented Jun 16, 2018

View reviewed changes

blegat and others added 5 commits June 16, 2018 13:45

Fix bridges tests

0e48a2a

Fix set! ConstraintFunction for mock optimizer

1ad6f61

Add more tests for default fallbacks

1e4479c

Add more documentation, tests for MultirowChange, and fix

1e91624

associated bug.

Refactor transformconstraint! to transform!. Currently this is not

300d57e

tested as transform! is not implemented in MockOptimizer

blegat reviewed Jun 17, 2018

View reviewed changes

mlubin reviewed Jun 17, 2018

View reviewed changes

odow added 2 commits June 18, 2018 10:29

Update modification docs with comments from @mlubin

8c02953

Address method ambiguity

3ffd5ab

blegat reviewed Jun 18, 2018

View reviewed changes

Refactor canset and set! to reduce method ambiguities

ec5b12a

Fix typo

b4c1ba8

odow mentioned this pull request Jun 19, 2018

Update LQOI to reflect refactoring of modifications in MOI JuliaOpt/LinQuadOptInterface.jl#29

Merged

4 tasks

Fix cantransform

cc41670

blegat reviewed Jun 19, 2018

View reviewed changes

Address comments by @blegat

8c9e53c

blegat mentioned this pull request Jun 19, 2018

Arbitrary constraints for UniversalFallback #397

Closed

Re-enable transform test

f55f777

blegat approved these changes Jun 19, 2018

View reviewed changes

mlubin approved these changes Jun 19, 2018

View reviewed changes

Address comments by @mlubin

622f009

joaquimg approved these changes Jun 20, 2018

View reviewed changes

mlubin merged commit bb80481 into master Jun 20, 2018

mlubin deleted the odow/new-modifications branch June 20, 2018 03:04

This was referenced Jun 20, 2018

Problems with transform! #398

Closed

Proposal: change API for modifications #360

Closed

This was referenced Jun 24, 2018

Compatibility with the new MOI modification API tkoolen/Parametron.jl#24

Closed

Update to new constraint modification API in MOI v0.4.0 osqp/OSQP.jl#19

Merged

odow mentioned this pull request Jun 25, 2018

Fix formatting of modification section #402

Merged

blegat mentioned this pull request Aug 9, 2018

Allow CachingOptimizer to set ConstraintSet #463

Closed


		### MultirowChange

		Finally, the last modification supports by MathOptInterface is the ability to


		## Objective Function

		canmodify(model::ModelLike, ::ObjectiveFunction, ::Type{M})::Bool where M<:AbstractFunctionModification

RFC: new modification syntax #388

RFC: new modification syntax #388

Uh oh!

Conversation

odow commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Changes

Tests

Blockers / future PR

Uh oh!

blegat commented Jun 14, 2018

Uh oh!

odow commented Jun 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blegat commented Jun 14, 2018

Uh oh!

codecov-io commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odow Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

odow commented Jun 14, 2018 •

edited

Loading

codecov-io commented Jun 14, 2018 •

edited

Loading

odow Jun 18, 2018 •

edited

Loading

blegat Jun 19, 2018 •

edited

Loading