Skip to content
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

Allow SetMapBridge to use bridge value #2509

Merged
merged 15 commits into from
Jun 22, 2024
Merged

Allow SetMapBridge to use bridge value #2509

merged 15 commits into from
Jun 22, 2024

Conversation

blegat
Copy link
Member

@blegat blegat commented May 28, 2024

When I decided that these functions would only depend on the type, it was for consistency for map_function because map_function clearly cannot depend on anything else given that it is used in bridge_constraint. However, you could also have a bridge that implements its own bridge_constraint, storing extra information then in the bridge to be able to implement inverse_map_function, etc... This is the case of the Sum-of-Squares bridge.

Needed for jump-dev/SumOfSquares.jl#355

src/Bridges/set_map.jl Outdated Show resolved Hide resolved
@blegat
Copy link
Member Author

blegat commented Jun 14, 2024

For map_function, there are two cases:

  1. The bridge can implement bridge_constraint and map_function(::AbstractBridge, ...) in which case in case the map depends on values stored in the bridge. map_function is used for setting the ConstraintPrimalStart so actually is not a huge priority to implement it
  2. The bridge implement map_function(::Type, ...) in which case the default fallback for bridge_constraint is enough.
    Since the second cases is useful as well, we shouldn't say map_function(::Type, ...) is legacy even if for the other ones I wrote in the docstring that they were legacy 🤔

@blegat blegat force-pushed the bl/set_map_type branch from 28fb249 to d101f91 Compare June 18, 2024 07:16
The default implementation of [`Variable.bridge_constraint`](@ref) uses
[`map_set`](@ref) with the bridge type so if this function is defined
on the bridge type, [`Variable.bridge_constraint`](@ref) does not need
to be implemented.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this comment. Variable.bridge_constraint does not exist.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean

function bridge_constraint(
BT::Type{<:MultiSetMapBridge{T,S1,G}},
model::MOI.ModelLike,
func::G,
set::S1,
) where {T,S1,G}
mapped_func = MOI.Bridges.map_function(BT, func)
mapped_set = MOI.Bridges.map_set(BT, set)
constraint = MOI.add_constraint(model, mapped_func, mapped_set)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just removed the comment

@odow odow merged commit 85a1db5 into master Jun 22, 2024
16 checks passed
@odow odow deleted the bl/set_map_type branch June 22, 2024 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants