Universal constraint fallback #415

blegat · 2018-06-30T17:35:28Z

Closes #397
Closes jump-dev/JuMP.jl#1152

blegat · 2018-07-08T13:30:52Z

Ready for review :)

codecov-io · 2018-07-08T13:43:14Z

Codecov Report

Merging #415 into master will increase coverage by 0.22%.
The diff coverage is 98.69%.

@@            Coverage Diff             @@
##           master     #415      +/-   ##
==========================================
+ Coverage   95.55%   95.78%   +0.22%     
==========================================
  Files          40       40              
  Lines        5088     5315     +227     
==========================================
+ Hits         4862     5091     +229     
+ Misses        226      224       -2

Impacted Files	Coverage Δ
src/Test/modellike.jl	`100% <100%> (ø)`	⬆️
src/Utilities/model.jl	`98.76% <96.55%> (+0.02%)`	⬆️
src/Utilities/universalfallback.jl	`99.43% <99.1%> (+0.61%)`	⬆️
src/Utilities/functions.jl	`99.35% <0%> (+1.44%)`	⬆️

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 851125c...bf61745. Read the comment docs.

mlubin · 2018-07-17T19:04:00Z

src/Utilities/model.jl

+    removevariable(f::MOI.AbstractFunction, s::MOI.AbstractSet, vi::MOI.VariableIndex)
+
+Return a tuple `(g, t)` representing the constraint `f`-in-`s` with the
+variable of index `vi` removed.


Unclear what it means to remove a variable from a constraint. Could you clarify?

mlubin · 2018-07-17T19:05:50Z

src/Utilities/model.jl

@@ -137,16 +142,48 @@ end
 MOI.canget(model::AbstractModel, ::MOI.Name) = true
 MOI.get(model::AbstractModel, ::MOI.Name) = model.name

+"""
+    newname(model::MOI.ModelLike, IdxT::Type{<:MOI.Index}, idx::MOI.Index, name::String)


Nit: IdxT could be IndexType. Another question for the style guide is whether we prefer idx or index.

Indeed, it would be nice to have a convention for that

mlubin · 2018-07-17T19:06:49Z

src/Utilities/model.jl

+end
+
+"""
+    setname(idxnames::Dict{<:MOI.Index, String}, namesidx::Dict{String, <:MOI.Index}, idx::MOI.Index, name::String, idxtype::Symbol)


Naming suggestion: index_to_name and name_to_index

mlubin · 2018-07-17T19:07:17Z

src/Utilities/model.jl

+    setname(idxnames::Dict{<:MOI.Index, String}, namesidx::Dict{String, <:MOI.Index}, idx::MOI.Index, name::String, idxtype::Symbol)
+
+Sets the name of the index `idx` to the name `name` in the maps `idxnames` and `namesidx`.
+If the name is already taken by an index different from `idx` then an error is thrown usin g `idxtype` to create a custom error message.


Typo: "usin g" -> "using"

mlubin · 2018-07-19T12:31:44Z

src/Utilities/model.jl

 end
 function _removevar!(constrs::Vector, vi::VI)
    for i in eachindex(constrs)
-        constrs[i] = _removevar(constrs[i]..., vi)
+        ci, f, s = constrs[i]
+        constrs[i] = (ci, removevariable(f, s, vi)...)
    end
    []


Nit: since you're touching this function, please add a return statement.

mlubin · 2018-07-19T12:35:15Z

src/Utilities/model.jl

+"""
+    newname(model::MOI.ModelLike, IndexType::Type{<:MOI.Index}, idx::MOI.Index, name::String)
+
+Return a `Bool` indicating whether `name` is not already used by an index of


The negative is confusing in this description. Maybe say "Returns a Bool indicating whether name is available to be used by an index of type IndexType (i.e., it's not already used)."

mlubin · 2018-07-19T12:37:37Z

src/Utilities/model.jl

@@ -137,16 +145,48 @@ end
 MOI.canget(model::AbstractModel, ::MOI.Name) = true
 MOI.get(model::AbstractModel, ::MOI.Name) = model.name

+"""
+    newname(model::MOI.ModelLike, IndexType::Type{<:MOI.Index}, idx::MOI.Index, name::String)


check_can_assign_name maybe? It's a bit confusing why this method should throw an error. What about returning 1 of 3 possible statuses: NameUnassigned, NameAssignedAndMatches, and NameMismatch?

If it does not throw an error, there won't be much code to share between the three places where this is used since we will have to branch on these three statuses. If we make typos in these branches we will introduce bugs and inconsistencies between the three places

mlubin · 2018-07-19T12:38:41Z

src/Utilities/model.jl

+    setname(index_to_names::Dict{<:MOI.Index, String}, names_to_index::Dict{String, <:MOI.Index}, idx::MOI.Index, name::String, idxtype::Symbol)
+
+Sets the name of the index `idx` to the name `name` in the maps `index_to_names` and `names_to_index`.
+If the name is already taken by an index different from `idx` then an error is thrown using `idxtype` to create a custom error message.


I'm looking at the code and don't see where the error is thrown.

mlubin · 2018-07-19T12:39:57Z

src/Utilities/universalfallback.jl

+critical constraints and attributes while still supporting other attributes
+with a small performance penalty. Note that `model` is unaware of constraints
+and attributes stored by `UniversalFallback` so this is not appropriate if
+`model` is an optimizer (for this reason, `optimize!` have not been


Nit: optimize! has not been implemented

mlubin · 2018-07-19T12:40:38Z

src/Utilities/universalfallback.jl

@@ -76,6 +127,29 @@ function MOI.get(uf::UniversalFallback, attr::Union{MOI.AbstractVariableAttribut
    end
 end

+function MOI.get(uf::UniversalFallback, attr::MOI.NumberOfConstraints{F, S}) where {F, S}
+    if MOI.supportsconstraint(uf.model, F, S)
+        MOI.get(uf.model, attr)


Nit: use return statements in these functions

mlubin · 2018-07-19T12:41:05Z

src/Utilities/universalfallback.jl

+end
+function MOI.get(uf::UniversalFallback, ::Type{CI}, name::String)
+    if MOI.canget(uf.model, CI, name)
+        MOI.get(uf.model, CI, name)


Nit: return statements

mlubin · 2018-07-19T12:41:32Z

src/Utilities/universalfallback.jl

-    MOI.canset(uf.model, attr, C)
+function MOI.canaddconstraint(uf::UniversalFallback, ::Type{F}, ::Type{S}) where {F<:MOI.AbstractFunction, S<:MOI.AbstractSet}
+    if MOI.supportsconstraint(uf.model, F, S)
+        MOI.canaddconstraint(uf.model, F, S)


Return statements

mlubin · 2018-07-19T12:42:07Z

test/universalfallback.jl

@@ -64,8 +65,12 @@ end

 struct UnknownOptimizerAttribute <: MOI.AbstractOptimizerAttribute end

+# A few constraint types are supported to test both the fallback and the


Nit: "A few" -> "Few"

To my Kiwi English, it seems weird to say "Few" here. To me, "A few" reads as "we've only implemented a couple of constraint types to test the functionality" whereas "Few" has a negative connotation and means "Not enough constraint types are supported".

SO suggests the following example:

I have few friends.

I have a few friends.

I suggested "few" because I understood that the comment was trying to say that the model supports fewer constraint types than usual so that it can test the fallback.

I meant "A few" in opposition of none otherwise only the fallback is tested

blegat changed the title ~~[WIP] Universal constraint fallback~~ Universal constraint fallback Jul 8, 2018

blegat added 11 commits July 13, 2018 11:03

Universal constraint fallback

aaaaca6

Reference discussion on JuMP issue

9a0764b

Fixes

edd73aa

Updates

a381111

More fixes

3ea3f36

Improve coverage and more fixes

75cc0dc

EqualTo -> LessThan

45f9dba

Add name tests

1d8f811

empty instead of creating new

a0b846d

Empty names maps in uf

c5aa361

Add tests

67ad86e

blegat force-pushed the bl/ufcon branch from f331dec to 67ad86e Compare July 13, 2018 10:11

blegat added 2 commits July 13, 2018 13:39

Fix canget for constraint attr

0a95448

Add tests

66b096c

mlubin reviewed Jul 17, 2018

View reviewed changes

Address comments

dd4cc14

mlubin reviewed Jul 19, 2018

View reviewed changes

Address comments

bf61745

mlubin approved these changes Jul 22, 2018

View reviewed changes

blegat merged commit 7862c11 into master Jul 22, 2018

blegat deleted the bl/ufcon branch August 28, 2018 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Universal constraint fallback #415

Universal constraint fallback #415

blegat commented Jun 30, 2018

blegat commented Jul 8, 2018

codecov-io commented Jul 8, 2018 •

edited

Loading

mlubin Jul 17, 2018

mlubin Jul 17, 2018

blegat Jul 18, 2018

mlubin Jul 17, 2018

mlubin Jul 17, 2018

mlubin Jul 19, 2018

mlubin Jul 19, 2018

mlubin Jul 19, 2018

blegat Jul 20, 2018

mlubin Jul 19, 2018

mlubin Jul 19, 2018

mlubin Jul 19, 2018

mlubin Jul 19, 2018

mlubin Jul 19, 2018

mlubin Jul 19, 2018

odow Jul 19, 2018

mlubin Jul 20, 2018

blegat Jul 20, 2018

		@@ -64,8 +65,12 @@ end

		struct UnknownOptimizerAttribute <: MOI.AbstractOptimizerAttribute end

		# A few constraint types are supported to test both the fallback and the

Universal constraint fallback #415

Universal constraint fallback #415

Conversation

blegat commented Jun 30, 2018

blegat commented Jul 8, 2018

codecov-io commented Jul 8, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 8, 2018 •

edited

Loading