-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bridges] add Constraint.FunctionConversionBridge #2235
Conversation
So I was simplifying things, when I realized that there was a better design ( I guess our options are:
|
I've moved some stuff around. We could leave removing |
We can add a new subtype in between to make it non-breaking. It still make sense to have one different bridge by output function. Here, we have a chain: variable -> affine -> quadratic -> nl struct FunctionConversionBridge{T,F,G} ... end
const ScalarFunctionizeBridge{T} = FunctionConversionBridge{T,ScalarAffineFunction}
...
``
so that implementing a bridge is one-line but the final function is still specified by the bridge.
We would have to add a `convertible` function to the API to make this work though. |
It should work if you define: function MOI.supports_constraint(
::Type{<:FunctionConversionBridge{T}},
::Type{PolyJuMP.Polynomial{T}},
::Type{<:MOI.AbstractScalarSet},
) where {T}
return true
end
function concrete_bridge_type(
::Type{<:FunctionConversionBridge{T}},
::Type{PolyJuMP.Polynomial{T}},
::Type{S},
) where {T,S<:MOI.AbstractScalarSet}
return FunctionConversionBridge{
T,
PolyJuMP.Polynomial{T},
MOI.ScalarNonlinearFunction,
S,
}
end |
That converts a polynomial to nonlinear but how do you create a bridge from nonlinear to polynomial ? |
You can't, because that's not possible in general?
This is a reasonable point. But it's pretty niche... |
This is exactly the goal of quadratic to RSOC. Because a conic solver does not support quadratic and this bridge might then you better try
My point with these two examples is that these are the tip of the iceberg pointing towards the fact that we're loosing some of the nice extensibility features of the bridges here. And this comes from the fact that we hardcode the next function to convert to in the chain of conversions. |
You could write a different bridge for SNF to polynomial. This bridge is a simple attempt at covering common use cases. I can't think of a situation in which you might like to disable ScalarAffine -> ScalarNonlinear, for example. |
It's actually as simple to cover all use cases. You PR is a big improvement over the current bridges and you does not need much change to become |
Once we have a function conversion like that, it's also easy to add something like function conversion from SNF to a |
This doesn't fix the deletion issue though? |
Shall we hold off on this PR for a bit then? I originally wanted to get it in for v1.19, but if |
Which deletion issue ? It's not going to stay, it's going to be replaced by ToScalarNonlinearBridge that converts from anything to SNF, not only quadratic |
Maybe we should call about this. I thought you wanted separate bridges for each change, rather than one that tried everything. |
IIUC, this PR is "Any-to-Any", the current status is "fixed function-to-fixed function" and I am suggesting "Any-to-fixed function" |
I'd say this PR is "F-to-G" for an explicit list of functions. The current status is "scalarquadratic to scalarnonlinear." I don't think we want an any-to-fix because it just leads to weird errors that are hard to interpret. |
I've reverted the name changes and the switch from This make the PR a lot smaller and easier to digest. The change required to support bridge-specific costs is actually very minor. Open questions would be:
|
It seems to be difficult to give rules. Note that in the future we could imagine the user changing the weights of a solver defined in MOI instead of having to do
I can see it being annoying, see #2235 (comment) |
We could change to |
We spoke offline and decided to switch to |
One last run of https://github.com/jump-dev/MathOptInterface.jl/actions/runs/5860994743 |
Can we add tests that certain important combinations like |
That's what this test is: although it's a little opaque. We could be more explicit. |
Ah, I missed that. The test is named |
I've tweaked the tests to check that we get the costs expected, and that the internal model is built as expected. |
Merging because I discussed this change with @blegat last night. Once merged, I'll update the prep for v1.19, and he can double check things before we tag the release. |
x-ref #2233