Allow setting scalar affine constraint functions via the modification API #37

rdeits · 2018-07-17T19:49:49Z

I'm not sure how to test this. There's a solve_func_scalaraffine_lessthan modification unit test in MOI.Test, but it doesn't seem to be run during the LQOI tests. I'd appreciate some help turning that on (and any other tests that are available).

codecov-io · 2018-07-17T19:54:31Z

Codecov Report

Merging #37 into master will increase coverage by 1.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #37      +/-   ##
==========================================
+ Coverage   76.27%   77.34%   +1.06%     
==========================================
  Files          13       13              
  Lines        1252     1311      +59     
==========================================
+ Hits          955     1014      +59     
  Misses        297      297

Impacted Files	Coverage Δ
src/constraints/vectoraffine.jl	`95.78% <100%> (+1.94%)`	⬆️
src/solver_interface.jl	`66.66% <100%> (+6.66%)`	⬆️
src/constraints/scalaraffine.jl	`95.89% <100%> (+0.89%)`	⬆️

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 4f75448...fc38846. Read the comment docs.

odow · 2018-07-17T22:47:30Z

I'd appreciate some help turning that on

The modification tests need to be added to LQOI. See GLPK for an example
https://github.com/JuliaOpt/GLPK.jl/blob/9a9e9f4ec6ac5bcc1b9b1b1e1a79fe9d2b43efe5/test/MOIWrapper.jl#L23-L25
The problem is that the mock solver in LQOI doesn't test the solution (since solve=false)

LinQuadOptInterface.jl/test/runtests.jl

Line 10 in d448b17

config = MOIT.TestConfig(solve = false)

so it isn't well tested. This needs to improve. I usually test by running GLPK and Gurobi on the changes.

@joaquimg is probably the person who understands the current state of the tests the best.

rdeits · 2018-07-17T22:49:31Z

Got it, thanks. I'll run glpk and gurobi with the modification tests on and open PRs to turn them on for those packages.

rdeits · 2018-07-18T01:39:35Z

By the way, jump-dev/Gurobi.jl#125 restricts LQOI to version [0.1, 0.2), but LQOI master is version 0.2+. Is that restriction necessary?

rdeits · 2018-07-18T01:45:26Z

PRs opened: jump-dev/GLPK.jl#56 jump-dev/Gurobi.jl#134

joaquimg · 2018-07-18T01:50:37Z

The problem with testing solutions is that one need to gather all the intermediary solutions (objective, variable primal and dual and constraint primal and dual) from some solver like Gurobi.
The mock solver is "ready"(but not tested) to receive these vectors as input.
Doing that for the "linear tests" is strainghtforward, not sure about the unit tests, but should not be hard.

rdeits · 2018-07-18T02:18:05Z

Hm, i just realized this isn't quite right, as I'm not handling a possible modification of the constant in the affine function. It doesn't seem to be possible to use a ScalarConstantChange in LQOI. Should I just replace the constraint set at the same time?

joaquimg · 2018-07-18T02:35:57Z

You are planning to modify the sparsity pattern of the constraint?
About performance... I would guess its faster to delete the constraint and add a new than query the old constraint and modify all coefficients.
ScalarConstantChange should be use when modifying few coefficients IMO.

Should I just replace the constraint set at the same time?

I would just replace the set.

rdeits · 2018-07-18T18:05:48Z

I suspect that we will often not change the sparsity pattern, but in general I want the code to produce the right result regardless. But you're right that actually getting the existing constraint function looks like it will be very expensive. We may just have to re-write our interface to use modify! instead of set!

tkoolen · 2018-07-18T19:48:44Z

Regarding testing modifications: I've been using the following in OSQP:

https://github.com/oxfordcontrol/OSQP.jl/blob/200811d9ba2ecafb07859764a271bcd3801c1b63/test/MathOptInterfaceOSQP.jl#L133-L176

Example usage:

https://github.com/oxfordcontrol/OSQP.jl/blob/200811d9ba2ecafb07859764a271bcd3801c1b63/test/MathOptInterfaceOSQP.jl#L249-L254

tkoolen · 2018-07-18T20:14:17Z

src/constraints/scalaraffine.jl

+    for term in replacement.terms
+        var = term.variable_index
+        chg = MOI.ScalarCoefficientChange{Float64}(var, term.coefficient)
+        MOI.modify!(m, CI, chg)


This also doesn't seem to support duplicate coefficients.

tkoolen · 2018-07-18T20:35:25Z

src/constraints/scalaraffine.jl

+        chg = MOI.ScalarCoefficientChange{Float64}(var, term.coefficient)
+        MOI.modify!(m, CI, chg)
+    end
+    change_rhs_coefficient!(m, m[CI], -replacement.constant)


Does this handle the constant encoded in the set properly (e.g. MOI.EqualTo(1.0))?

tkoolen · 2018-07-18T20:46:41Z

Some thoughts:

there should probably be an LQOI.change_matrix_coefficients! (plural) to reduce the number of low level API calls
at least for Gurobi, it seems that sparsity pattern changes are already handled internally. It's possible that simply zeroing everything (as opposed to just the currently-nonzero coefficients) is already efficient, so that getting the constraint function from the solver is not necessary.
something like OSQP's MatrixModificationCache is probably needed to handle duplicate coefficients in an (at least algorithmically) efficient way (note that this is actually a moderately slow part of the OSQP interface due to lots of Dict lookups).

odow · 2018-08-06T19:46:31Z

What's the status of this? Is it necessary for the next release of LQOI? Or can it wait?

rdeits · 2018-08-06T19:48:52Z

Sorry, I haven't had time to vet this properly, and I think its handling of scalar affine functions is incorrect. It's on my todo list but not quite at the top.

If you're OK with leaving this open, I'll let you know when it's ready for review.

rdeits · 2018-08-06T19:49:52Z

But to answer your question, no, this should not be necessary for the next release. It's just adding a new feature.

rdeits · 2018-08-07T18:25:29Z

@odow will this change also be rendered moot by your switch to using bridges here? I'm just trying to decide whether to keep this as a local modification for my own work or to actually put in the time to finish it correctly.

odow · 2018-08-07T21:25:39Z

I would keep it as a local change for now. Other than you, no one is clamoring for this change, so it doesn't make sense to spend a lot of time on it.

rdeits · 2018-08-18T01:42:35Z

Hey @odow this is actually becoming a bit more pressing for my work. I believe I've fixed the bug with RHS coefficients that was previously in this branch, and the lack of this feature is currently blocking tkoolen/Parametron.jl#75

The scalar part of this PR is tested by jump-dev/GLPK.jl#56 and the vector part is tested by tkoolen/Parametron.jl#75

Would you be willing to consider merging this, even if it will eventually be replaced by the bridges?

rdeits · 2018-08-19T17:18:15Z

This time for sure...

I think this is ready for review (although we should merge #53 first, since this PR depends on that one). There are now enough tests that I'm reasonably confident it's doing the right thing.

Unfortunately, this code is going to be very inefficient, particularly for GLPK. The problem is that every call to GLPK's implementation of change_matrix_coefficient! requires loading an entire row from the solver, performing the update, and then setting that entire row. We have to do that load and store for every single element in the constraint function. This inefficiency would be completely resolved by having GLPK implement MOI.set(..., ::ConstraintFunction) itself, which could be done at a later date.

odow · 2018-08-19T22:48:29Z

I've merged #53. Will take a look at this

odow · 2018-08-19T22:49:40Z

src/constraints/scalaraffine.jl

@@ -196,18 +196,65 @@ function MOI.modify!(model::LinQuadOptimizer, index::LCI{S}, change::MOI.ScalarC
    change_matrix_coefficient!(model, row, column, change.new_coefficient)
 end

+function _replace_with_matching_sparsity!(model::LinQuadOptimizer, previous::Linear, replacement::Linear, row)


I know the rest of the code doesn't, but can we add docstrings for these new functions to remind ourselves of why they are needed and what they do?

odow · 2018-08-19T22:52:34Z

src/constraints/scalaraffine.jl

+    rows = fill(row, length(replacement.terms))
+    cols = [model.variable_mapping[t.variable_index] for t in replacement.terms]
+    coefs = MOIU.coefficient.(replacement.terms)
+    change_matrix_coefficients!(model, rows, cols, coefs)


Do these account for duplicate coefficients? (Edit: looks from below that the inputs are canonicalized. All the more reason for docstrings.)

I've made a note of that assumption in the various docstrings

odow · 2018-08-19T22:55:57Z

src/constraints/scalaraffine.jl

+    end
+    change_rhs_coefficient!(model, row, get_rhs(model, row) - (replacement.constant - previous.constant))
+    model.constraint_constant[model[CI]] = replacement.constant
+    nothing


Nit: return or return nothing

odow · 2018-08-19T22:56:59Z

src/constraints/vectoraffine.jl

@@ -76,6 +76,62 @@ function MOI.modify!(model::LinQuadOptimizer, index::VLCI{<: VecLinSets},
    end
 end

+_constant(f::Linear) = f.constant
+_constant(f::VecLin) = f.constants


Are there MOIU functions for this?

There are, but they're likewise _-prefixed so I didn't want to use them externally. MOIU does have constant() but it unnecessarily allocates a vector for scalar functions.

codecov-io · 2018-08-20T02:46:43Z

Codecov Report

Merging #37 into master will increase coverage by 0.83%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #37      +/-   ##
==========================================
+ Coverage   76.27%   77.11%   +0.83%     
==========================================
  Files          13       13              
  Lines        1252     1315      +63     
==========================================
+ Hits          955     1014      +59     
- Misses        297      301       +4

Impacted Files	Coverage Δ
src/constraints/vectoraffine.jl	`95.78% <100%> (+1.94%)`	⬆️
src/constraints/scalaraffine.jl	`95.89% <100%> (+0.89%)`	⬆️
src/solver_interface.jl	`63.15% <100%> (+3.15%)`	⬆️
src/constraints/singlevariable.jl	`75.8% <0%> (ø)`	⬆️
src/solve.jl	`0% <0%> (ø)`	⬆️

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 4f75448...1cb27fa. Read the comment docs.

odow

Thanks @rdeits

joaquimg · 2018-08-28T14:33:20Z

src/constraints/scalaraffine.jl

@@ -183,7 +183,7 @@ function MOI.get(model::LinQuadOptimizer, ::MOI.ConstraintFunction, index::LCI{<
        model.variable_references[columns],
        coefficients
    )
-    Linear(terms, -model.constraint_constant[row])
+    Linear(terms, model.constraint_constant[row])


This was not touched by tests?

Nope. But I actually already made that fix (and added a test) in https://github.com/JuliaOpt/LinQuadOptInterface.jl/pull/53/files#diff-c1cce28d94a3c050507f8711d20ca7b2R186

joaquimg · 2018-08-28T14:34:18Z

Looks good to go. I just added one question about the sign change.

tkoolen · 2018-09-04T14:56:32Z

Ready to go?

rdeits · 2018-09-04T20:33:00Z

Yeah, I think this should be ready. Can we merge it?

tkoolen

Actually, indices should be changed to Compat.axes in a couple of places for 1.0 compatibility.

tkoolen · 2018-09-04T21:30:43Z

src/constraints/vectoraffine.jl

+in canonical form.
+"""
+function _matching_sparsity_pattern(f1::T, f2::T) where {T <: Union{Linear, VecLin}}
+    indices(f1.terms) == indices(f2.terms) || return false


indices -> Compat.axes.

tkoolen · 2018-09-04T21:31:05Z

src/solver_interface.jl

+some solver interfaces may offer more efficient implementations.
+"""
+function change_matrix_coefficients!(m, rows, cols, coefs)
+    @boundscheck((indices(rows) == indices(cols) == indices(coefs)) || throw(DimensionMismatch()))


indices -> Compat.axes.

odow · 2018-09-04T22:51:37Z

Merging this. We can rebase #60 on top of this and address any other compat issues.

rdeits mentioned this pull request Jul 17, 2018

Compatibility with LinQuadOptInterface solvers (like Gurobi and GLPK) tkoolen/Parametron.jl#31

Closed

This was referenced Jul 18, 2018

enable constraint function modification test jump-dev/GLPK.jl#56

Merged

enable constraint function modification test jump-dev/Gurobi.jl#134

Closed

tkoolen reviewed Jul 18, 2018

View reviewed changes

rdeits force-pushed the rd/set-constraint-func branch from 939876c to 99ce57f Compare July 25, 2018 15:16

tkoolen mentioned this pull request Jul 25, 2018

MOI wrapper: problems with updating constraint functions jump-dev/GLPK.jl#58

Closed

odow mentioned this pull request Jul 31, 2018

Tag new release #40

Closed

4 tasks

rdeits force-pushed the rd/set-constraint-func branch from 03271c3 to 9389980 Compare August 1, 2018 16:19

rdeits changed the title ~~allow setting scalar affine constraint functions via the modification API~~ WIP: allow setting scalar affine constraint functions via the modification API Aug 6, 2018

rdeits force-pushed the rd/set-constraint-func branch from 9389980 to 06c8289 Compare August 17, 2018 22:30

rdeits mentioned this pull request Aug 18, 2018

WIP: Update to MOI 0.5 tkoolen/Parametron.jl#75

Merged

2 tasks

fix constant when getting constraint function

1da042f

rdeits added 10 commits August 19, 2018 12:44

bug fixes

13ebf8a

more hacking at constraint replacement

a2f0aeb

cleaner interface for vector constraints

8b14337

fix RHS bugs and re-enable scalar constraint function modification

507eb60

WIP: more constant fixes

029aced

closer

b0c79ed

clean up

77d01fd

start adding tests for issue JuliaOpt#32

f8b4378

clean up tests

7536c3e

more testing

2af6a7c

rdeits force-pushed the rd/set-constraint-func branch from bc4d5c0 to 2af6a7c Compare August 19, 2018 16:44

rdeits changed the title ~~WIP: allow setting scalar affine constraint functions via the modification API~~ Allow setting scalar affine constraint functions via the modification API Aug 19, 2018

odow reviewed Aug 19, 2018

View reviewed changes

rdeits added 2 commits August 19, 2018 22:45

docstrings

fc38846

explicit return

cf80e48

odow requested a review from joaquimg August 20, 2018 04:28

odow approved these changes Aug 20, 2018

View reviewed changes

joaquimg reviewed Aug 28, 2018

View reviewed changes

joaquimg approved these changes Aug 29, 2018

View reviewed changes

tkoolen reviewed Sep 4, 2018

View reviewed changes

indices() -> Compat.axes()

1cb27fa

odow merged commit 68fffb1 into JuliaOpt:master Sep 4, 2018

odow mentioned this pull request Sep 4, 2018

[WIP] Add solutions for tests #57

Merged

Allow setting scalar affine constraint functions via the modification API #37

Allow setting scalar affine constraint functions via the modification API #37

Conversation

rdeits commented Jul 17, 2018 • edited Loading

codecov-io commented Jul 17, 2018 • edited Loading

Codecov Report

odow commented Jul 17, 2018

rdeits commented Jul 17, 2018

rdeits commented Jul 18, 2018

rdeits commented Jul 18, 2018

joaquimg commented Jul 18, 2018

rdeits commented Jul 18, 2018

joaquimg commented Jul 18, 2018

rdeits commented Jul 18, 2018

tkoolen commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkoolen commented Jul 18, 2018

odow commented Aug 6, 2018

rdeits commented Aug 6, 2018

rdeits commented Aug 6, 2018

rdeits commented Aug 7, 2018

odow commented Aug 7, 2018

rdeits commented Aug 18, 2018

rdeits commented Aug 19, 2018

odow commented Aug 19, 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

codecov-io commented Aug 20, 2018 • edited Loading

Codecov Report

odow left a 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

joaquimg commented Aug 28, 2018

tkoolen commented Sep 4, 2018

rdeits commented Sep 4, 2018

tkoolen left a 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 Sep 4, 2018

rdeits commented Jul 17, 2018 •

edited

Loading

codecov-io commented Jul 17, 2018 •

edited

Loading

codecov-io commented Aug 20, 2018 •

edited

Loading