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

[Bridges] fix adjoint functions in SetDotInverseScalingBridge #2264

Merged
merged 2 commits into from
Sep 7, 2023

Conversation

odow
Copy link
Member

@odow odow commented Sep 6, 2023

Potential fix for #2262 (comment).

I don't really understand what's going here, but it makes sense that the adjoint should be the opposite of the forward map? That's what it is for SetDotScalingBridge, so I think this is just a typo.

Seemed to fix SDPA, so I'll re-run solver-tests on this branch: https://github.com/jump-dev/MathOptInterface.jl/actions/runs/6103555292

@odow odow mentioned this pull request Sep 6, 2023
6 tasks
@odow
Copy link
Member Author

odow commented Sep 7, 2023

Mosek still broken, but because you explicitly coded the bridge type into a test:
https://github.com/jump-dev/MosekTools.jl/blob/6624a98dcef6b4d6619111e9b40b8afb05b40bae/test/runtests.jl#L264

@blegat
Copy link
Member

blegat commented Sep 7, 2023

This is weird. The adjoint is the transpose and the transpose if a Diagonal should be the matrix itself

@blegat
Copy link
Member

blegat commented Sep 7, 2023

We should add a test that sets and gets ConstraintDualStart in the bridge test, it should clarify

@odow
Copy link
Member Author

odow commented Sep 7, 2023

We should add a test that sets and gets ConstraintDualStart in the bridge test, it should clarify

There is a test. But it just tests that the round trip preserves. So it doesn't help if it gets the wrong value... 😆

@odow
Copy link
Member Author

odow commented Sep 7, 2023

This is weird. The adjoint is the transpose and the transpose if a Diagonal should be the matrix itself

Isn't the SetDotInverseScalingBridge just the inverse of this? So it makes sense they'd be swapped.

# Since the map is a diagonal matrix `D`, it is symmetric so one would initially
# expect `adjoint_map_function` to be the same as `map_function`. However, the
# scalar product for the scaled PSD cone is `<x, y>_2 = x'y` but the scalar
# product for the PSD cone additionally scales the offdiagonal entries by `2`
# hence by `D^2` so `<x, y>_1 = x'D^2y`.
# So `<Dx, y>_2 = <x, D^(-1)y>_1` hence the adjoint of `D` is its inverse!
function MOI.Bridges.adjoint_map_function(
::Type{<:SetDotScalingBridge{T,S}},
func,
) where {T,S}
return _inverse_scale(T, S, func)
end
function MOI.Bridges.inverse_adjoint_map_function(
::Type{<:SetDotScalingBridge{T,S}},
func,
) where {T,S}
return _scale(T, S, func)
end

@blegat
Copy link
Member

blegat commented Sep 7, 2023

It's good that I left a comment to explain this to my future self 😆 . So this fix is indeed correct because the adjoint is not the transpose. Can you add a test that don't just check the round trip ?

@odow odow merged commit 90cd379 into master Sep 7, 2023
17 checks passed
@odow odow deleted the od/fix-set-dot-scaling branch September 7, 2023 23:34
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