Remove SingleVariable

[blegat]

Just a proof of concept at the moment

Closes #1472

[odow]

What do we think of this? I took a look, and migrating all of the calls is a lot of work

[blegat]

A lot of work in MOI or in the whole ecosystem or both ?

[odow]

In MOI and the ecosystem. There's thousands of changes. I tried scripting the changes

create_regex(regex) = regex => s -> match(regex, s)[1]

regexes = [
    create_regex(r"MOI\.SingleVariable\(([a-zA-Z\.\[\]0-9]+?)\)"),
    create_regex(r"MOI\.SingleVariable\.\(([a-zA-Z\.\[\]0-9]+?)\)"),
    create_regex(r"SingleVariable\(([a-zA-Z\.\[\]0-9]+?)\)"),
    create_regex(r"SingleVariable\.\(([a-zA-Z\.\[\]0-9]+?)\)"),
    "::NOI.SingleVariable" => "::MOI.VariableIndex",
    "::SingleVariable" => "::VariableIndex",
    "{MOI.SingleVariable" => "{MOI.VariableIndex",
    "{SingleVariable" => "{VariableIndex",
    "MOI.SingleVariable," => "MOI.VariableIndex,",
]
for (root, dirs, files) in walkdir(".")
    for file in files
        if !(endswith(file, ".jl") || endswith(file, ".md"))
            continue
        end
        filename = joinpath(root, file)
        file = read(filename, String)
        if !occursin("SingleVariable", file)
            continue
        end
        for (regex, f) in regexes
            file = replace(file, regex => f)
        end
        write(filename, file)
    end
end

which takes the number of changes required from thousands to hundreds...

[blegat]

Yes, I agree it's a huge fixed cost of time to pay now. But it will only get worse with time and I think it might be saving time over time that will overweight this cost.

[odow]

I'm still not convinced that this is necessary. Can you point to code that would be significantly simplified if we removed SingleVariable? And why the benefits of this change out-weights the cost?

[blegat]

I would not say that one code will be significantly simplified but rather than many codes will be simplified.
One example is:
https://github.com/jump-dev/SumOfSquares.jl/blob/f75d7510fca6c03de7039d532b76be0e441a976e/src/sos_polynomial.jl#L94
https://github.com/jump-dev/SumOfSquares.jl/blob/f75d7510fca6c03de7039d532b76be0e441a976e/src/Bridges/Constraint/sos_polynomial.jl#L44
I create variables and then a gram matrix from the vector of variables. The gram matrix element should implement +, *, ... so I convert them into SingleVariable. However, in other places, I need to use the VariableIndex. For this reason, add_gram_matrix returns both the gram matrix and the variables which is redundant. So in the bridge, I only store the variables and when I need to create the gram matrice, I recreate it:
https://github.com/jump-dev/SumOfSquares.jl/blob/f75d7510fca6c03de7039d532b76be0e441a976e/src/Bridges/Constraint/sos_polynomial.jl#L183-L184
If the gram matrices contained variable indices, I would probably just return the gram matrices and store that.

Here are example where many small complications are due to SingleVariable is:
https://github.com/lanl-ansi/Juniper.jl/blob/3a30ee5298d0dae426348da840c994eda901b985/src/fpump.jl#L22
https://github.com/lanl-ansi/Juniper.jl/blob/3a30ee5298d0dae426348da840c994eda901b985/src/fpump.jl#L114
https://github.com/blegat/SwitchOnSafety.jl/blob/1aa6dff4106d0f65c943dfb983defc20b2d3f04b/src/balanced_real_polytope.jl#L61-L62
This could go on and on, this is basically your first 4 regex. By the way, the fact that these 4 regex match thousands of lines means that it's a thousands of lines code that had to be complicated because of SingleVariable!

Here is an example where I tried to have a code working both for JuMP and MOI model:
https://github.com/dionysos-dev/Dionysos.jl/blob/8e30fe31147fb59134899122acb48eb612919d50/src/bemporad_morari.jl
It does not work as for MOI, you have no equivalent of VariableRef that works for all case.
I chose SingleVariable:
https://github.com/dionysos-dev/Dionysos.jl/blob/8e30fe31147fb59134899122acb48eb612919d50/src/bemporad_morari.jl#L96
But then MOI.get(model, MOI.VariablePrimal(), variable) does not work for MOI so I had to define some helper _value:
https://github.com/dionysos-dev/Dionysos.jl/blob/8e30fe31147fb59134899122acb48eb612919d50/src/bemporad_morari.jl#L94-L95

[odow]

I would not say that one code will be significantly simplified but rather than many codes will be simplified.
One example is:
https://github.com/jump-dev/SumOfSquares.jl/blob/f75d7510fca6c03de7039d532b76be0e441a976e/src/sos_polynomial.jl#L94
https://github.com/jump-dev/SumOfSquares.jl/blob/f75d7510fca6c03de7039d532b76be0e441a976e/src/Bridges/Constraint/sos_polynomial.jl#L44
I create variables and then a gram matrix from the vector of variables. The gram matrix element should implement +, *, ... so I convert them into SingleVariable. However, in other places, I need to use the VariableIndex. For this reason, add_gram_matrix returns both the gram matrix and the variables which is redundant. So in the bridge, I only store the variables and when I need to create the gram matrice, I recreate it:
https://github.com/jump-dev/SumOfSquares.jl/blob/f75d7510fca6c03de7039d532b76be0e441a976e/src/Bridges/Constraint/sos_polynomial.jl#L183-L184
If the gram matrices contained variable indices, I would probably just return the gram matrices and store that.

Okay. It's reasonable to be annoyed at returning both x and SingleVariable(x).

Here are example where many small complications are due to SingleVariable is:
https://github.com/lanl-ansi/Juniper.jl/blob/3a30ee5298d0dae426348da840c994eda901b985/src/fpump.jl#L22
https://github.com/lanl-ansi/Juniper.jl/blob/3a30ee5298d0dae426348da840c994eda901b985/src/fpump.jl#L114
https://github.com/blegat/SwitchOnSafety.jl/blob/1aa6dff4106d0f65c943dfb983defc20b2d3f04b/src/balanced_real_polytope.jl#L61-L62

Are you worried about the extra allocation of the mx and nx vectors? Because you could just wrap the usages of nx[i] in a SingleVariable on-demand.

Here is an example where I tried to have a code working both for JuMP and MOI model:
https://github.com/dionysos-dev/Dionysos.jl/blob/8e30fe31147fb59134899122acb48eb612919d50/src/bemporad_morari.jl
It does not work as for MOI, you have no equivalent of VariableRef that works for all case.
I chose SingleVariable:
https://github.com/dionysos-dev/Dionysos.jl/blob/8e30fe31147fb59134899122acb48eb612919d50/src/bemporad_morari.jl#L96
But then MOI.get(model, MOI.VariablePrimal(), variable) does not work for MOI so I had to define some helper _value:
https://github.com/dionysos-dev/Dionysos.jl/blob/8e30fe31147fb59134899122acb48eb612919d50/src/bemporad_morari.jl#L94-L95

This is a pretty extreme edge-case (making it work for JuMP and MOI). And you seem to have sorted it out by a couple of helper functions.

This could go on and on, this is basically your first 4 regex. By the way, the fact that these 4 regex match thousands of lines means that it's a thousands of lines code that had to be complicated because of SingleVariable!

I'm not saying that removing SingleVariable won't lead to cleaner code. I'm saying that the cost of doing so outweighs the benefits.

Rewriting thousands of lines of code to save a few helper functions in a couple of packages doesn't seem like it's worth the effort. We also have to go through all of the documentation, the paper would be out-dated, all talks we gave would be out-dated. It's not the same as changing the order of some fields (or renaming them).

Can we leave as-is for MOI 1.0 and think about it for MOI 2.0?

[dourouc05]

For the record, here is what I wrote on Gitter:

From a purely conceptual point of view, having the distinction between an index and a function is paramount. In practice, though, I don't think it's useful at all.

Would there be any performance benefits of getting rid of SingleVariable? It would account for one fewer layer of abstraction, so it shouldn't hurt, but I guess Julia is already optimising the code quite a lot.

[odow]

In general, there are no performance benefits to removing it. In some cases you might need to allocate a new array if you need to pass an array of SingleVariable instead of VariableIndex, but that will never be more than a tiny part of the total cost so isn't really a consideration.

The trade-off here is: is it worth changing thousands of lines of code across every package in the ecosystem for the small benefit?

[freemin7]

I am for this change as it seems to make things conceptually simpler and makes future efforts in the ecosystem less technical.

Edit: I have one barely half working NLP solver where the MOI interface is still private. So weigh my opinions with respect to my contributions

[blegat]

Rewriting thousands of lines of code to save a few helper functions in a couple of packages doesn't seem like it's worth the effort. We also have to go through all of the documentation, the paper would be out-dated, all talks we gave would be out-dated. It's not the same as changing the order of some fields (or renaming them).

I agree it's a lot. But the usability change is quite substantial.
Every single use of MOI is affected.
If you want to create a SingleVariable constraint, you are affected.
If you want to create a ScalarAffineFunction, you are affected if you want to use *, +, ...
It's not big issues but when you add them up...
I'm waiting to see what other people think.

Can we leave as-is for MOI 1.0 and think about it for MOI 2.0?

I'd say it's now or never. Doing it in MOI 2.0 is even worse.

Would there be any performance benefits of getting rid of SingleVariable?

I don't think it is noticeable, you can also write your code in a way such that it does not affect you. But you might need to complicate things a bit.

[odow]

But the usability change is quite substantial.

Only if you want to write MOI constraints algebraically, and you don't want to first wrap variables in SingleVariable.

I'm waiting to see what other people think.

[blegat]

Only if you want to write MOI constraints algebraically, and you don't want to first wrap variables in SingleVariable.

I agree but I suspect that people don't do it algebraically because if the usability issue. Some may even try, get errors and don't realize that they could just use SingleVariable. It's a bit of a vicious circle :)

I can take care of updating packages in the jump-dev Github org.

I'm waiting to see what other people think.

For the record, @joehuchette was also in favor of this during the JuMP-dev call of July.

[joehuchette]

For the record, @joehuchette was also in favor of this during the JuMP-dev call of July.

I've found this a relatively minor, but persistent, annoyance when trying to build constraints algebraically.

[matbesancon]

After following the discussion for a while, I'm also in favour of this, it's worth delaying 0.10 and have things a bit simpler

[odow]

We can discuss during this week's call. But this is the remaining blocker for MOI 0.10.

[matbesancon]

should we close this now that it's overtaken by #1559 ?