RFC: modifications #1351

odow · 2018-06-19T08:26:03Z

I have updated this to the new JuMP syntax.

TODO

docs

~~We discussed having a proof of concept for JuMP-dev of constraint modification.~~

Is this the right way to do it? I haven't delved into the new JuMP code at all so have no idea if this is the "correct" way to implement it or where it should live etc.

At the very least, it works!*

*Requires my PRs to LQOI, GLPK and MOI

codecov · 2018-06-19T08:48:14Z

Codecov Report

Merging #1351 into master will increase coverage by 0.12%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #1351      +/-   ##
=========================================
+ Coverage   89.47%   89.6%   +0.12%     
=========================================
  Files          27      27              
  Lines        3621    3790     +169     
=========================================
+ Hits         3240    3396     +156     
- Misses        381     394      +13

Impacted Files	Coverage Δ
src/constraints.jl	`95.12% <100%> (+0.25%)`	⬆️
src/macros.jl	`88.38% <0%> (+1.18%)`	⬆️

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 906e982...0532ed9. Read the comment docs.

blegat · 2018-06-19T08:53:24Z

examples/basic.jl

+const MOI = MathOptInterface
+function setrhs(c, rhs)
+    moi = c.m.moibackend
+    MOI.set!(moi, MOI.ConstraintSet(), c.index, MOI.set_type(c.index)(rhs))


It is cleaner to do index(c) rather than c.index

blegat · 2018-06-19T09:07:43Z

have no idea if this is the "correct" way to implement it

Looks like the correct way to me :)

where it should live

I'd say we define a setconstant function in JuMP.

For ScalarAffineFunction-in-* and VectorAffineFunction-in-*, it modifies the constant of the function
For *-in-GreaterThan, *-in-LessThan and *-in-EqualTo, it modifies the constant in the set
For ScalarAffineFunction-in-GreaterThan, ... there is an ambiguity which needs to be resolved by modifying the constant in the set.

It would probably be simpler if we always stored the constant in the function for GreaterThan, .. sets except for SingleVariable, that way we would have had the simpler rule

For ScalarAffineFunction-in-* and VectorAffineFunction-in-*, it modifies the constant of the function
For SingleVariable-in-GreaterThan, SingleVariable-in-LessThan and SingleVariable-in-EqualTo, it modifies the constant in the set

By the way if we didn't have SingleVariable we wouldn't have needed a field in these sets and could have simply defined GreaterThanZero, ...
I am not suggesting to remove SingleVariable but simply wondering if we shouldn't have always stored the constant in the function when the function supports constants and when the function does not support it, it should be stored in the set.

mlubin · 2018-06-19T15:27:59Z

examples/basic.jl

+const MOI = MathOptInterface
+function setrhs(c, rhs)
+    moi = c.m.moibackend
+    MOI.set!(moi, MOI.ConstraintSet(), c.index, MOI.set_type(c.index)(rhs))


The JuMP call should look at least like:

MOI.set!(m, MOI.ConstraintSet(), c, MOI.LessThan(rhs))

Users shouldn't need to extract the moibackend or the MOI index (even though it's easy).

mlubin · 2018-06-19T15:28:29Z

examples/basic.jl

+const MOI = MathOptInterface
+function setrhs(c, rhs)
+    moi = c.m.moibackend
+    MOI.set!(moi, MOI.ConstraintSet(), c.index, MOI.set_type(c.index)(rhs))


MOI.set_type(c.index)(rhs) is confusing for an example. Just use a concrete type.

odow · 2018-10-12T21:47:53Z

Oops. I managed to drop the old commit instead of reverting it. It was totally superseded by this anyway.

odow · 2018-10-12T21:49:08Z

I need this so I can go around changing right-hand sides and coefficients in the constraint matrix.

I'm not sure what the best approach is. We either provide JuMP-specific functions (as done here), or somehow make the MOI modification layer accessible.

mlubin · 2018-10-13T01:19:42Z

src/constraints.jl

+Note that prior to this step, JuMP will move all variable terms onto the
+left-hand side, and all constant terms onto the right-hand side. For example,
+given a constraint `2x + 1 <= 2`, `JuMP.set_constant(c, 3)` will create the
+constraint `2x <= 3`, not `2x + 1 <= 3`.


This ambiguity could be resolved by providing add_to_rhs instead of set_rhs. What do you think?

It seems safer to define add_constant which is mathematically defined as adding a constant in the function side but which adds the opposite value in the rhs for scalar constraint.
set_rhs is ambiguous for Interval and does not work for vector constraints.

Revisiting #470 Maybe the simplest thing is to add an extra variable and then use JuMP.fix(x, value). It is a whole lot cleaner with less issues. In which case, this is just a documentation issue.

The issue with add_to_rhs is that it makes sense, but how many people would actually use it?

If you're not excited about add_to_rhs we can drop RHS modifications from this PR. set_coefficient looks fine.

src/constraints.jl

blegat · 2018-10-13T06:56:39Z

src/constraints.jl

+
+Note that prior to this step, JuMP will move all variable terms onto the
+left-hand side, and all constant terms onto the right-hand side. For example,
+given a constraint `2x + 1 <= 2`, `JuMP.set_constant(c, 3)` will create the


Is is called set_constant or set_rhs ?

Oops, I changed back and forth a few times and missed this one.

mlubin · 2018-10-14T15:43:36Z

src/constraints.jl

+Note that prior to this step, JuMP will move all variable terms onto the
+left-hand side, and all constant terms onto the right-hand side. For example,
+given a constraint `2x + 1 <= 2`, `JuMP.set_constant(c, 3)` will create the
+constraint `2x <= 3`, not `2x + 1 <= 3`.


If you're not excited about add_to_rhs we can drop RHS modifications from this PR. set_coefficient looks fine.

mlubin · 2018-10-14T15:44:40Z

src/constraints.jl

+`JuMP.set_coefficient(c, x, 4)` will create the constraint `4x <= 2`.
+
+
+```jldoctest


Could you drop set_coefficient somewhere in the docs so that the doctest gets run?

blegat reviewed Jun 19, 2018

View reviewed changes

mlubin reviewed Jun 19, 2018

View reviewed changes

odow mentioned this pull request Jun 24, 2018

use Ref instead of Array when getting Gurobi parameters jump-dev/Gurobi.jl#131

Merged

Add simple modifications

26acb04

odow force-pushed the odow/modifications branch from 80584e1 to 26acb04 Compare October 12, 2018 21:45

odow changed the title ~~Proof of Concept: modifications~~ RFC: modifications Oct 12, 2018

mlubin reviewed Oct 13, 2018

View reviewed changes

blegat reviewed Oct 13, 2018

View reviewed changes

Respond to comments

001ed31

mlubin reviewed Oct 14, 2018

View reviewed changes

Remove set_rhs

0532ed9

mlubin approved these changes Oct 14, 2018

View reviewed changes

odow merged commit 37eb05c into master Oct 15, 2018

odow mentioned this pull request Nov 7, 2018

Support MOI modifications #1183

Closed

odow deleted the odow/modifications branch December 30, 2018 02:24

joaquimg mentioned this pull request Mar 8, 2019

Modify RHS (and define RHS) #1890

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: modifications #1351

RFC: modifications #1351

odow commented Jun 19, 2018 •

edited

codecov bot commented Jun 19, 2018 •

edited

blegat Jun 19, 2018

blegat commented Jun 19, 2018 •

edited

mlubin Jun 19, 2018

mlubin Jun 19, 2018

odow commented Oct 12, 2018

odow commented Oct 12, 2018

mlubin Oct 13, 2018

blegat Oct 13, 2018

odow Oct 13, 2018

mlubin Oct 14, 2018

blegat Oct 13, 2018

odow Oct 13, 2018

mlubin Oct 14, 2018

mlubin Oct 14, 2018

		`JuMP.set_coefficient(c, x, 4)` will create the constraint `4x <= 2`.


		```jldoctest

RFC: modifications #1351

RFC: modifications #1351

Conversation

odow commented Jun 19, 2018 • edited

TODO

codecov bot commented Jun 19, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

blegat commented Jun 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odow commented Oct 12, 2018

odow commented Oct 12, 2018

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

odow commented Jun 19, 2018 •

edited

codecov bot commented Jun 19, 2018 •

edited

blegat commented Jun 19, 2018 •

edited