[Bridges] fix supports for VariableIndex: Take II

[odow]

A fix for the fix in #1991.

This is actually a super subtle part of the bridging system, and we don't have good test coverage. I think I understand the space of possible outcomes, and I've tested with Ipopt, SCS, and SDPA etc.

So before merging:

• Add better tests

[blegat]

I'll take a look

[blegat]

Suppose you have a solver that supports

• Adding variables constrained on creation in S or T
• Adding a constraint VAF-in-T

You add variables on creation to S
Then you add a constraint on this vector of variables in T. This is bridged into VAF-in-T.
Here, is_bridged(b, T) is false with a LazyBridgeOptimizer.

There are cases in which both might be possible, it depends whether it was constrained on creation or not. In that case, supports should return true if one of the two cases is true because there is a chance that it is supported.

[odow]

I'm going around in circles. Is there a way to tell if a VectorOfVariables was added by add_constrained_variables? Not really just from the type, right?

[odow]

So I think we actually have a much bigger issue that means we can't get this right.

The SingleBridgeOptimizer tests are failing for things like flip_sign.jl, because if you add a VectorOfVariables-in-Nonnegatives constraint, the solver supports the constraint and the attribute. But if you add constrained variables in Nonnegatives, the solver force bridges (even though the model supports it) and the bridge doesn't support the attribute.

But, we can't distinguish whether to return true or false, because given a constraint index we don't know whether it was added as a constraint or a constrained variable.

[blegat]

Exactly, given just the type, you cannot know how all constraint of that type will be dealt with.You need to consider all possible cases and return true if at least one case supports it

given a constraint index we don't know whether it was added as a constraint or a constrained variable.

Given a constraint index you can know that but not given just the types

[blegat]

Note that, as shown by the reduce_bridged function that was removed in #1937, there are 3 places depending on the same logic so if there was something wrong with this one, we should also update the other 2 places accordingly

[odow]

So I took a look at this, introducing the || instead of && following your comment #1937 (comment). And it breaks more tests than it fixes. We either give a false negative, or a false positive. I think for now, let's keep this behavior, since it fixes a bug in Ipopt and all current tests remain passing.

[blegat]

Let's a least keep either an open issue or open PR then

[odow]

Okay. I'll merge and open an issue