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

Fix querying dual of symmetric equality constraints #3797

Merged
merged 1 commit into from
Aug 5, 2024
Merged

Conversation

odow
Copy link
Member

@odow odow commented Aug 2, 2024

Closes #3796

I need to think about whether there is a better option. I don't think we should just fix the default reshape_vector because we don't need to modify anything for the primal? Just the dual?

Copy link

codecov bot commented Aug 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.40%. Comparing base (bf662d4) to head (39b99bc).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3797      +/-   ##
==========================================
+ Coverage   98.39%   98.40%   +0.01%     
==========================================
  Files          44       44              
  Lines        5907     5956      +49     
==========================================
+ Hits         5812     5861      +49     
  Misses         95       95              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@odow
Copy link
Member Author

odow commented Aug 2, 2024

Okay, this looks like the problem. We never had any tests for this.

@blegat
Copy link
Member

blegat commented Aug 2, 2024

You can use JuMP.dual_shape.
This is similar to what we do in
https://github.com/jump-dev/MathOptInterface.jl/blob/a15b67fe47ad20caf316eb1bba0369a46ceb5a34/src/Bridges/Constraint/bridges/square.jl#L401-L411
The rule in dual is to multiply by the adjoint of the linear transformation we did.
In case the matrix was symmetric, we assume that the transformation was taking the average of the two off-diagonal entries hence the transpose would set half the dual to each entry. But, because the triangular dot product multiplies by 2, the adjoint does not divide by 2 and just set the same value to both off-diagonal.
In here, because Zeros has the standard dot product, the adjoint and transpose are the same so assuming that the transformation takes the average, this PR is the adjoint. Could you add a small explanation in the comments explaining that we are simply doing the adjoint (assuming we used the average).

@odow
Copy link
Member Author

odow commented Aug 2, 2024

You can use JuMP.dual_shape

I don't think so, or at least, not for SymmetricMatrixShape and HermitianMatrixShape, because these are a special case of us interpreting Matrix -in- Zeros. If we implemented dual_shape it would also modify things like Matrix in PSD.

Or we need a new matrix shape just for these Matrix-in-Zeros constraints.

@blegat
Copy link
Member

blegat commented Aug 2, 2024

Or we need a new matrix shape just for these Matrix-in-Zeros constraints.

Yes, maybe we need to do that. I'm in favor

@odow odow mentioned this pull request Aug 4, 2024
1 task
@odow
Copy link
Member Author

odow commented Aug 4, 2024

Name suggestions for the primal and dual shapes?

SquareSymmetricMatrixShape and SquareSymmetricMatrixDualShape?

@odow odow changed the title WIP: fix querying dual of symmetric equality constraints Fix querying dual of symmetric equality constraints Aug 4, 2024
@odow
Copy link
Member Author

odow commented Aug 4, 2024

Okay, instead of adding more shapes, I've added needs_adjoint_dual kwarg.

Copy link
Member

@blegat blegat left a comment

Choose a reason for hiding this comment

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

I agree a keyword argument is simpler

@odow odow merged commit 95da051 into master Aug 5, 2024
11 checks passed
@odow odow deleted the od/sym-dual branch August 5, 2024 21:01
@araujoms
Copy link
Contributor

araujoms commented Aug 9, 2024

I'm afraid this PR introduced a bug: before if I tried to constrain A == B for Symmetric A and Hermitian B it would give me an error. Now it silently fails.

using JuMP
using LinearAlgebra
import Hypatia
import Clarabel
import Ket
import Random

function dual_test(d)
    Random.seed!(1)
    ρ = Ket.random_state(Float64, d^2, 1)

    model = Model()
    @variable(model, σ[1:d^2, 1:d^2] in PSDCone())
    @variable(model, λ)
    noisy_state = Hermitian+ λ * I(d^2))
    @constraint(model, witness_constraint, σ == noisy_state)
    @objective(model, Min, λ)
    @constraint(model, Ket.partial_transpose(σ, 2, [d, d]) in PSDCone())

    set_optimizer(model, Hypatia.Optimizer)
    #set_optimizer(model, Clarabel.Optimizer)
    optimize!(model)

    W = dual(witness_constraint)
    return W
end

@blegat
Copy link
Member

blegat commented Aug 9, 2024

Oops, can you open an issue so that we can track this ?

@odow
Copy link
Member Author

odow commented Aug 9, 2024

Now it silently fails

No, now it silently succeeds 😄

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.

Incorrect dual variable extracted from equality constraint between Hermitian matrices
3 participants