jump-dev / MathOptInterface.jl

A data structure for mathematical optimization problems
http://jump.dev/MathOptInterface.jl/
Other
387 stars 87 forks source link

Fix issue #2452 #2464

Closed odow closed 5 months ago

odow commented 5 months ago

Fixes #2452

https://github.com/jump-dev/MathOptInterface.jl/actions/runs/8548954452 https://github.com/jump-dev/MathOptInterface.jl/actions/runs/8550055568 https://github.com/jump-dev/MathOptInterface.jl/actions/runs/8550389750

odow commented 5 months ago

So this seemed to do the job, but I'm not sure I can tell you why.

blegat commented 5 months ago

What's happening for example in the test you added ?

x >= 1 -> y = x - 1 with y nonneg
2x = 3 -> 2y = 1
func = 2y
f = 2x - 2
g = 2y

In this example, MOI.constant(g) is zero so it should be the same before and after the change you've made. What's the catch ?

odow commented 5 months ago

Ah, but it is actually:

x >= 1 -> y = x - 1 with y nonneg
2x = 3 -> 2y = 1
func = 2x
f = 2x
g = 2y + 2

because when we try to get func = 2y, it calls: https://github.com/jump-dev/MathOptInterface.jl/blob/606ba2ecae8d97eef79cee89a6d14351126b73b0/src/Bridges/Constraint/bridges/vectorize.jl#L229-L240 But MOI.get(model, attr, bridge.vector_constraint) returns the "unbridged" version of the function instead of the bridged. (Because it doesn't have a way to indicate that it will handle the variable bridges afterwards.)

The problematic method which does the unbridging is: https://github.com/jump-dev/MathOptInterface.jl/blob/606ba2ecae8d97eef79cee89a6d14351126b73b0/src/Bridges/bridge_optimizer.jl#L1380-L1404

I tried adding some new RawConstraintFunction attribute, but then I got confused trying to plumb it all together.

odow commented 5 months ago

Nightly failure is https://github.com/JuliaLang/julia/pull/53896#discussion_r1555087951

odow commented 5 months ago

Okay. I think I understand this a lot better now. I've simplified the implementation and everything looks green.

blegat commented 5 months ago

I think we still need the if-else with the call_in_context. Here is why. You have the user model and the solver model. But you might also have models in between. If a user variable x gets variable-bridged by a bridge bxy into an intermediate variable y which is then variable-bridged by a bridge byz into a solver variable z then you have an intermediate model inter for which the variable is y. All constraints created by bxy are in the context of inter and the variable they use is y. So if a constraint created by bxy is bridged by a constraint bridge and this constraint bridge tries to get MOI.ConstraintSet(), it should get the ConstraintFunction in terms of y. Maybe keeping the original way of querying func but then doing g = bridged_function(b, func) works ?

odow commented 5 months ago

Can you construct a test that demonstrates the current behavior is wrong? Otherwise, I vote we keep this as-is. I don't really understand the ins and outs of call-in-context.

odow commented 5 months ago

So perhaps if we kept the call_in_context, then we need the MOI.constant(g) - MOI.constant(f), but just getting ::MOI.ConstraintFunction deals with all the different constants.

I've added a test that the user can't add a function with a constant if there are variable bridges.

I don't know well this was ever tested. It didn't, for example, have a check for supports_shift_constant

odow commented 5 months ago

The implementation of what is going on at each level is certainly non-trivial.

klamike commented 5 months ago

Thank you for your help on this issue!

Unfortunately, I think this PR does not fully fix #2452. It seems that using the latest master branch, the result of the pure MOI MWE is now correct but the original MWE still gives the same (incorrect) result. Interestingly, using remove_bridge to remove VectorizeBridge still seems to fix the original MWE.

odow commented 5 months ago

I've re-opened #2452. I'm sure I tested it at some point and it was fixed...

odow commented 5 months ago

Okay. I've dug deeper, and now I have no confidence that this was actually the right fix. It just fixed the surface level get/set, but it incorrectly modified the inner model.