[WIP] Scalar affine function reloaded #1238

matbesancon · 2021-02-15T23:24:14Z

This PR is a first take on #863

Before we get into yet another scope creep, I think it is reasonable to try this only on SAF for now.

This comes from few observations:

A scalar linear function can be viewed as a collection of terms, or as a pair of collections (coefficients, variables). There is no right way to represent it
Most (all?) solvers need the second representation and end up rebuilding the two vectors.
MOI may be passed a collection of terms (it may make more sense when parsing JuMP expressions) or the tuple. Having an alternative representation can avoid allocating memory twice (once to convert to SAF, one to convert back to two vectors).

I started by adding the second representation ScalarAffineColumnFunction and realized most of the code could be factorized with an abstract type AbstractScalarAffineFunction{T}.

Still to-do:

Bridge from one to the other: not sure this is needed in addition to conversion?
Documentation update on some points
Tests

blegat · 2021-02-16T15:28:10Z

src/functions.jl

+Represents a scalar-valued affine function ``a^T x + b``.
+Unlike `ScalarAffineFunction`, it maintains the coefficients and variables in separate vectors.
+"""
+mutable struct ScalarAffineColumnFunction{T} <: AbstractScalarAffineFunction{T}


Why calling it column ?

Excellent question, I couldn't find a non-terrible name so I just picked one arbitrary for now. Actually this could be more ScalarAffineRowFunction because you represent the row of coefficients

odow · 2021-02-17T02:11:52Z

A bridge would be good. Then solvers could, if they choose, drop support for ScalarAffineFunction and use this one instead. If it turns out to be much better, then we could transition to JuMP making these instead.

So I guess my wishlist is also: benchmarks to see the difference of using this.

mlubin · 2021-02-17T02:14:43Z

Agreed, benchmarks should drive this change.

If it turns out to be much better, then why keep the old format?

matbesancon · 2021-02-17T10:04:01Z

If it turns out to be much better, then why keep the old format?

this could be a longer-term goal. I agree that a bridge will be a first way to start a replacement

blegat · 2021-02-17T13:13:24Z

We could also have

struct GenericScalarAffineFunction{T, VT<:AbstractVector{MOI.ScalarAffineTerm{T}}}
    terms::VT
    constant::T
end
const ScalarAffineFunction{T} = GenericScalarAffineFunction{T, Vector{MOI.ScalarAffineTerm{T}}}
struct ZipTermVector{T} <: AbstractVector{MOI.ScalarAffineTerm{T}}
    coefficients::Vector{T}
    variables::Vector{MOI.VariableIndex}
end

If ZipTermVector implements the AbstractVector API, then most of the code should work with both types of ScalarAffineFunction while allowing solver to directly pick the vector of coefficients if the right type of vector of terms is used.
This would close #525

matbesancon · 2021-02-20T14:49:56Z

@blegat's design proposal seems to fit the needs of the PR and would result in less changes, I'll change things accordingly.
The only awkward thing is that solvers will dispatch on the subtype VT in:

GenericScalarAffineFunction{T, VT}

blegat · 2021-02-23T15:04:55Z

You can still define const ScalarAffineColumnFunction{T} = GenericScalarAffineFunction{T, ...} to facilitates dispatch

matbesancon · 2021-03-04T21:44:49Z

I'll let you appreciate the awfully cryptic error messages:

ERROR: LoadError: LoadError: cannot assign a value to variable Core.Integer from module MathOptInterface
Stacktrace:
 [1] top-level scope at /home/mbesancon/julia_projects/MathOptInterface.jl/src/sets.jl:667
 [2] include(::Function, ::Module, ::String) at ./Base.jl:380
 [3] include at ./Base.jl:368 [inlined]
 [4] include(::String) at /home/mbesancon/julia_projects/MathOptInterface.jl/src/MathOptInterface.jl:1
 [5] top-level scope at /home/mbesancon/julia_projects/MathOptInterface.jl/src/MathOptInterface.jl:136
 [6] include(::Function, ::Module, ::String) at ./Base.jl:380
 [7] include(::Module, ::String) at ./Base.jl:368
 [8] top-level scope at none:2
 [9] eval at ./boot.jl:331 [inlined]
 [10] eval(::Expr) at ./client.jl:467
 [11] top-level scope at ./none:3
in expression starting at /home/mbesancon/julia_projects/MathOptInterface.jl/src/sets.jl:667
in expression starting at /home/mbesancon/julia_projects/MathOptInterface.jl/src/MathOptInterface.jl:136
ERROR: LoadError: Failed to precompile MathOptInterface [b8f27783-ece8-5eb3-8dc8-9495eed66fee] to /home/mbesancon/.julia/compiled/v1.5/MathOptInterface/tyub8_cF1HQ.ji.

odow · 2021-03-04T21:48:50Z

Presumably it's the use of ::Integer in src/functions.jl which conflicts with the MOI set MOI.Integer. Perhaps just use ::Core.Integer instead?

src/functions.jl

matbesancon · 2021-03-04T22:01:10Z

Indeed yes, fixed it and added comments in last commits. I now how more expected failing tests

matbesancon · 2021-03-04T22:08:45Z

All the failures now are in Bridges, looking like follows:

 [1] top-level scope at /home/mbesancon/julia_projects/MathOptInterface.jl/test/Bridges/lazy_bridge_optimizer.jl:490
 [2] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Test/src/Test.jl:1115
 [3] top-level scope at /home/mbesancon/julia_projects/MathOptInterface.jl/test/Bridges/lazy_bridge_optimizer.jl:483
 [4] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Test/src/Test.jl:1190
Interval constraint: Test Failed at /home/mbesancon/julia_projects/MathOptInterface.jl/test/Bridges/lazy_bridge_optimizer.jl:500
  Expression: debug_string(MOIB.debug_supports_constraint, F, S) == MOI.Utilities.replace_acronym("`MOI.ScalarAffineFunction{$(T)}`-in-`MOI.Interval{$(T)}` constraints are not supported and cannot be bridged into supported constrained variables and constraints. See details below:\n [1] constrained variables in `MOI.GreaterThan{$(T)}` are not supported because no added bridge supports bridging it.\n   Cannot add free variables and then constrain them because free variables are bridged but no functionize bridge was added.\n [2] constrained variables in `MOI.LessThan{$(T)}` are not supported because no added bridge supports bridging it.\n   Cannot add free variables and then constrain them because free variables are bridged but no functionize bridge was added.\n [3] constrained variables in `MOI.Interval{$(T)}` are not supported because no added bridge supports bridging it.\n   Cannot add free variables and then constrain them because free variables are bridged but no functionize bridge was added.\n (1) `MOI.ScalarAffineFunction{$(T)}`-in-`MOI.Interval{$(T)}` constraints are not supported because:\n   Cannot use `$(MOIB.Constraint.SplitIntervalBridge{T, MOI.ScalarAffineFunction{T}, MOI.Interval{T}, MOI.GreaterThan{T}, MOI.LessThan{T}})` because:\n   (2) `MOI.ScalarAffineFunction{$(T)}`-in-`MOI.GreaterThan{$(T)}` constraints are not supported\n   (3) `MOI.ScalarAffineFunction{$(T)}`-in-`MOI.LessThan{$(T)}` constraints are not supported\n   Cannot use `$(MOIB.Constraint.ScalarSlackBridge{T, MOI.ScalarAffineFunction{T}, MOI.Interval{T}})` because:\n   [3] constrained variables in `MOI.Interval{$(T)}` are not supported\n (2) `MOI.ScalarAffineFunction{$(T)}`-in-`MOI.GreaterThan{$(T)}` constraints are not supported because:\n   Cannot use `$(MOIB.Constraint.ScalarSlackBridge{T, MOI.ScalarAffineFunction{T}, MOI.GreaterThan{T}})` because:\n   [1] constrained variables in `MOI.GreaterThan{$(T)}` are not supported\n (3) `MOI.ScalarAffineFunction{$(T)}`-in-`MOI.LessThan{$(T)}` constraints are not supported because:\n   Cannot use `$(MOIB.Constraint.ScalarSlackBridge{T, MOI.ScalarAffineFunction{T}, MOI.LessThan{T}})` because:\n   [2] constrained variables in `MOI.LessThan{$(T)}` are not supported\n")
   Evaluated: "`MOI.GenericScalarAffineFunction{Int64,Array{MOI.ScalarAffineTerm{Int64},1}}`-in-`MOI.Interval{Int64}` constraints are not supported and cannot be bridged into supported constrained variables and constraints. See details below:\n [1] constrained variables in `MOI.GreaterThan{Int64}` are not supported because no added bridge supports bridging it.\n   Cannot add free variables and then constrain them because free variables are bridged but no functionize bridge was added.\n [2] constrained variables in `MOI.LessThan{Int64}` are not supported because no added bridge supports bridging it.\n   Cannot add free variables and then constrain them because free variables are bridged but no functionize bridge was added.\n [3] constrained variables in `MOI.Interval{Int64}` are not supported because no added bridge supports bridging it.\n   Cannot add free variables and then constrain them because free variables are bridged but no functionize bridge was added.\n (1) `MOI.GenericScalarAffineFunction{Int64,Array{MOI.ScalarAffineTerm{Int64},1}}`-in-`MOI.Interval{Int64}` constraints are not supported because:\n   Cannot use `MOIB.Constraint.SplitIntervalBridge{Int64,MOI.GenericScalarAffineFunction{Int64,Array{MOI.ScalarAffineTerm{Int64},1}},MOI.Interval{Int64},MOI.GreaterThan{Int64},MOI.LessThan{Int64}}` because:\n   (2) `MOI.GenericScalarAffineFunction{Int64,Array{MOI.ScalarAffineTerm{Int64},1}}`-in-`MOI.GreaterThan{Int64}` constraints are not supported\n   (3) `MOI.GenericScalarAffineFunction{Int64,Array{MOI.ScalarAffineTerm{Int64},1}}`-in-`MOI.LessThan{Int64}` constraints are not supported\n   Cannot use `MOIB.Constraint.ScalarSlackBridge{Int64,MOI.GenericScalarAffineFunction{Int64,Array{MOI.ScalarAffineTerm{Int64},1}},MOI.Interval{Int64}}` because:\n   [3] constrained variables in `MOI.Interval{Int64}` are not supported\n (2) `MOI.GenericScalarAffineFunction{Int64,Array{MOI.ScalarAffineTerm{Int64},1}}`-in-`MOI.GreaterThan{Int64}` constraints are not supported because:\n   Cannot use `MOIB.Constraint.ScalarSlackBridge{Int64,MOI.GenericScalarAffineFunction{Int64,Array{MOI.ScalarAffineTerm{Int64},1}},MOI.GreaterThan{Int64}}` because:\n   [1] constrained variables in `MOI.GreaterThan{Int64}` are not supported\n (3) `MOI.GenericScalarAffineFunction{Int64,Array{MOI.ScalarAffineTerm{Int64},1}}`-in-`MOI.LessThan{Int64}` constraints are not supported because:\n   Cannot use `MOIB.Constraint.ScalarSlackBridge{Int64,MOI.GenericScalarAffineFunction{Int64,Array{MOI.ScalarAffineTerm{Int64},1}},MOI.LessThan{Int64}}` because:\n   [2] constrained variables in `MOI.LessThan{Int64}` are not supported\n" == "`MOI.ScalarAffineFunction{Int64}`-in-`MOI.Interval{Int64}` constraints are not supported and cannot be bridged into supported constrained variables and constraints. See details below:\n [1] constrained variables in `MOI.GreaterThan{Int64}` are not supported because no added bridge supports bridging it.\n   Cannot add free variables and then constrain them because free variables are bridged but no functionize bridge was added.\n [2] constrained variables in `MOI.LessThan{Int64}` are not supported because no added bridge supports bridging it.\n   Cannot add free variables and then constrain them because free variables are bridged but no functionize bridge was added.\n [3] constrained variables in `MOI.Interval{Int64}` are not supported because no added bridge supports bridging it.\n   Cannot add free variables and then constrain them because free variables are bridged but no functionize bridge was added.\n (1) `MOI.ScalarAffineFunction{Int64}`-in-`MOI.Interval{Int64}` constraints are not supported because:\n   Cannot use `MOIB.Constraint.SplitIntervalBridge{Int64,MOI.GenericScalarAffineFunction{Int64,Array{MOI.ScalarAffineTerm{Int64},1}},MOI.Interval{Int64},MOI.GreaterThan{Int64},MOI.LessThan{Int64}}` because:\n   (2) `MOI.ScalarAffineFunction{Int64}`-in-`MOI.GreaterThan{Int64}` constraints are not supported\n   (3) `MOI.ScalarAffineFunction{Int64}`-in-`MOI.LessThan{Int64}` constraints are not supported\n   Cannot use `MOIB.Constraint.ScalarSlackBridge{Int64,MOI.GenericScalarAffineFunction{Int64,Array{MOI.ScalarAffineTerm{Int64},1}},MOI.Interval{Int64}}` because:\n   [3] constrained variables in `MOI.Interval{Int64}` are not supported\n (2) `MOI.ScalarAffineFunction{Int64}`-in-`MOI.GreaterThan{Int64}` constraints are not supported because:\n   Cannot use `MOIB.Constraint.ScalarSlackBridge{Int64,MOI.GenericScalarAffineFunction{Int64,Array{MOI.ScalarAffineTerm{Int64},1}},MOI.GreaterThan{Int64}}` because:\n   [1] constrained variables in `MOI.GreaterThan{Int64}` are not supported\n (3) `MOI.ScalarAffineFunction{Int64}`-in-`MOI.LessThan{Int64}` constraints are not supported because:\n   Cannot use `MOIB.Constraint.ScalarSlackBridge{Int64,MOI.GenericScalarAffineFunction{Int64,Array{MOI.ScalarAffineTerm{Int64},1}},MOI.LessThan{Int64}}` because:\n   [2] constrained variables in `MOI.LessThan{Int64}` are not supported\n"

odow · 2021-03-04T22:34:11Z

The bridge errors are just printing.

More serious is this

Testing /home/runner/work/MathOptInterface.jl/MathOptInterface.jl/test/Bridges/Constraint/slack.jl
311
Scalar slack: Error During Test at /home/runner/work/MathOptInterface.jl/MathOptInterface.jl/test/Bridges/Constraint/slack.jl:14
312
  Got exception outside of a @test
313
  UndefVarError: VT not defined
314
  Stacktrace:
315
   [1] MathOptInterface.GenericScalarAffineFunction(::VT, ::Float64) where VT<:Array{MathOptInterface.ScalarAffineTerm{Float64},1} at /home/runner/work/MathOptInterface.jl/MathOptInterface.jl/src/functions.jl:80
316
   [2] modify_function at /home/runner/work/MathOptInterface.jl/MathOptInterface.jl/src/Utilities/functions.jl:973 [inlined]
317
   [3] _modifyconstr at /home/runner/work/MathOptInterface.jl/MathOptInterface.jl/src/Utilities/model.jl:68 [inlined]
318
   [4] _modify(::Array{Tuple{MathOptInterface.ConstraintIndex{MathOptInterface.GenericScalarAffineFunction{Float64,Array{MathOptInterface.ScalarAffineTerm{Float64},1}},MathOptInterface.EqualTo{Float64}},MathOptInterface.GenericScalarAffineFunction{Float64,Array{MathOptInterface.ScalarAffineTerm{Float64},1}},MathOptInterface.EqualTo{Float64}},1}, ::MathOptInterface.ConstraintIndex{MathOptInterface.GenericScalarAffineFunction{Float64,Array{MathOptInterface.ScalarAffineTerm{Float64},1}},MathOptInterface.EqualTo{Float64}}, ::Int64, ::MathOptInterface.ScalarConstantChange{Float64}) at /home/runner/work/MathOptInterface.jl/MathOptInterface.jl/src/Utilities/model.jl:76

matbesancon · 2021-03-05T09:56:41Z

Using type printing for tests is not robust.

Julia 1.6:

julia> MOI.ScalarAffineFunction{Float64}
MathOptInterface.GenericScalarAffineFunction{Float64, Vector{MathOptInterface.ScalarAffineTerm{Float64}}}

Julia 1.5:

julia> MOI.ScalarAffineFunction{Float64}
MathOptInterface.GenericScalarAffineFunction{Float64,Array{MathOptInterface.ScalarAffineTerm{Float64},1}}

The hacky path:tm: would be to override show for the type

matbesancon · 2021-03-05T10:12:03Z

pinging @blegat, not sure what a good future-proof solution would be

odow · 2021-03-05T19:23:53Z

You should interpolate the type into the string that is being checked against.

Instead of "MOI.ScalarAffineFunction{Float64}", use "$(MOI.ScalarAffineFunction{Float64})".

This reverts commit 5aaa7df.

matbesancon · 2021-03-11T12:49:02Z

the issue is that string interpolation with $ replaces MOI with MathOptInterface

odow · 2021-03-11T19:19:31Z

Looks like you need to update these sorts of lines

MathOptInterface.jl/test/Bridges/lazy_bridge_optimizer.jl

Line 352 in e0f8b05

    
            (1) `MOI.VectorOfVariables`-in-`MOI.PositiveSemidefiniteConeSquare` constraints are bridged (distance 1) by $(MOIB.Constraint.SquareBridge{T,MOI.VectorOfVariables,MOI.ScalarAffineFunction{T},MOI.PositiveSemidefiniteConeTriangle,MOI.PositiveSemidefiniteConeSquare}).

matbesancon · 2021-08-13T07:47:06Z

@odow some update when updating these lines, this was a pain because the alias printing behavior changed with julia 1.6, so we could accommodate the string tests to 1.6 or to 1.0

blegat · 2021-08-13T09:57:26Z

Note that it needs to be exported for the alias to work, see #1521 (comment)

matbesancon · 2021-08-25T20:16:38Z

@blegat that's not helping our case if we don't want to export things and still keep the SAF alias correctly? In case getting this alias to work is troublesome, this might mean we would prefer making this (slightly) breaking change before a 1.0

matbesancon · 2021-08-25T20:16:58Z

@odow related to the 1.x label

odow · 2021-08-26T04:37:54Z

I think we can count printing changes as non-breaking (that's what Julia has done between minor versions), so I'm not worried about that. Keeping this as a 1.x item.

odow · 2022-11-15T22:10:47Z

What's the status of this? Potentially close as stale? I think there's still appetite for exploring alternative representations, but the cost of transitioning is high.

matbesancon · 2022-11-15T22:18:36Z

we can close as stale, I do think there is a pain point in having to unpack inner structures into flat (coefficient, indices) structs but I don't have the bandwidth for it right now

odow · 2022-11-15T22:27:27Z

I do think there is a pain point in having to unpack inner structures into flat

Yeah. One lesson I think we can learn from MOI is that vector-of-structs was a bad idea. For a linear function, you want

struct ScalarAffineFunction{T}
    constant::T
    terms::Dict{VariableIndex,T}
end

# or

struct ScalarAffineFunction{T}
    constant::T
    variables::Vector{VariableIndex}
    coefficients::Vector{T}
end

I get that iterating over a Vector{ScalarAffineTerm{T}} was easier, but it's not that much harder to iterate of the two vectors, and you could even create an iterator type that made it simple. But having access to the vector of coefficients directly is useful.

odow · 2022-11-15T22:28:15Z

Closing as stale. Will leave #863 open.

matbesancon added 3 commits February 16, 2021 00:09

first draft

cd46a39

remove union

ddabe1d

remove union for quadratic conversion

df344ba

blegat reviewed Feb 16, 2021

View reviewed changes

fix methods

9557248

Merge branch 'master' into new-scalar-func

03c406a

matbesancon added 3 commits March 4, 2021 19:40

Merge branch 'master' into new-scalar-func

c433c37

begin replacement

e177ef4

rename with GenericScalarAffine

b75f0ff

odow reviewed Mar 4, 2021

View reviewed changes

src/functions.jl Outdated Show resolved Hide resolved

src/functions.jl Outdated Show resolved Hide resolved

fixed Integer bug and constructors

c1c1936

fixed specifier SAF to specific generic function

1820501

constructor from other affine function

5aaa7df

matbesancon added 2 commits March 11, 2021 13:37

correct interpolation

40b9ba3

Revert "constructor from other affine function"

2f5d42d

This reverts commit 5aaa7df.

fix first part interpolation

e1a4a0b

matbesancon added 5 commits March 11, 2021 20:41

merge conflict

dbd1d54

undo VAF rename

cc41eff

adapt tests

cdc5867

change SAF to generic in utilities

dad7b86

fix conflict

1fad066

odow added this to the MOI 1.x milestone Aug 12, 2021

odow closed this Nov 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Scalar affine function reloaded #1238

[WIP] Scalar affine function reloaded #1238

matbesancon commented Feb 15, 2021

blegat Feb 16, 2021

matbesancon Feb 16, 2021

odow commented Feb 17, 2021

mlubin commented Feb 17, 2021

matbesancon commented Feb 17, 2021

blegat commented Feb 17, 2021

matbesancon commented Feb 20, 2021

blegat commented Feb 23, 2021

matbesancon commented Mar 4, 2021

odow commented Mar 4, 2021

matbesancon commented Mar 4, 2021

matbesancon commented Mar 4, 2021

odow commented Mar 4, 2021

matbesancon commented Mar 5, 2021

matbesancon commented Mar 5, 2021

odow commented Mar 5, 2021

matbesancon commented Mar 11, 2021

odow commented Mar 11, 2021

matbesancon commented Aug 13, 2021

blegat commented Aug 13, 2021

matbesancon commented Aug 25, 2021

matbesancon commented Aug 25, 2021

odow commented Aug 26, 2021

odow commented Nov 15, 2022

matbesancon commented Nov 15, 2022

odow commented Nov 15, 2022

odow commented Nov 15, 2022

[WIP] Scalar affine function reloaded #1238

[WIP] Scalar affine function reloaded #1238

Conversation

matbesancon commented Feb 15, 2021

blegat Feb 16, 2021

Choose a reason for hiding this comment

matbesancon Feb 16, 2021

Choose a reason for hiding this comment

odow commented Feb 17, 2021

mlubin commented Feb 17, 2021

matbesancon commented Feb 17, 2021

blegat commented Feb 17, 2021

matbesancon commented Feb 20, 2021

blegat commented Feb 23, 2021

matbesancon commented Mar 4, 2021

odow commented Mar 4, 2021

matbesancon commented Mar 4, 2021

matbesancon commented Mar 4, 2021

odow commented Mar 4, 2021

matbesancon commented Mar 5, 2021

matbesancon commented Mar 5, 2021

odow commented Mar 5, 2021

matbesancon commented Mar 11, 2021

odow commented Mar 11, 2021

matbesancon commented Aug 13, 2021

blegat commented Aug 13, 2021

matbesancon commented Aug 25, 2021

matbesancon commented Aug 25, 2021

odow commented Aug 26, 2021

odow commented Nov 15, 2022

matbesancon commented Nov 15, 2022

odow commented Nov 15, 2022

odow commented Nov 15, 2022