jump-dev / JuMP.jl

Modeling language for Mathematical Optimization (linear, mixed-integer, conic, semidefinite, nonlinear)
http://jump.dev/JuMP.jl/
Other
2.22k stars 393 forks source link

Fix *(::AbstractJuMPScalar{<:Real}, ::Hermitian) to return Hermitian #3695

Closed odow closed 7 months ago

odow commented 7 months ago

Closes #3694

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.35%. Comparing base (a9b5ff9) to head (9b0eaee).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3695 +/- ## ========================================== - Coverage 98.36% 98.35% -0.02% ========================================== Files 43 43 Lines 5699 5707 +8 ========================================== + Hits 5606 5613 +7 - Misses 93 94 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

araujoms commented 7 months ago

Could you also do the Symmetric case? It's actually much easier because you don't need to test whether the scalar is real.

odow commented 7 months ago

This one we can fix in MA: https://github.com/jump-dev/MutableArithmetics.jl/pull/268

araujoms commented 7 months ago

I've just realized that with this definition of multiplication you are generating hidden undefs in the lower triangular, that is, you are relying on the bug JuliaLang/julia#52895 being fixed, which I think is only the case for Julia 1.11 onwards. So the following code will fail for all previous version of Julia:

using JuMP
prob = Model()
@variable(prob,x)
m = x*LinearAlgebra.Hermitian(randn(2,2))
m+m
odow commented 7 months ago

Perhaps we should close this then. The currently behavior is correct. The new PR would introduce the risk of incorrect behavior due to bugs in Base.

If JuMP generates a Matrix, you can always explicitly wrap it in Hermitian.

It's really hard to uniformly support all of LinearAlgebra, SparseArrays, MutableArithmetics, and JuMP.

araujoms commented 7 months ago

Or you could just return instead

LinearAlgebra.Hermitian(Matrix(LinearAlgebra.Hermitian(B, c)),c)

This gets rids of the undefs, so no bugs are possible, you keep the performance gain, and don't require the users to always rewrap everything in Hermitian.

araujoms commented 7 months ago

Thanks a lot for solving all of the problems I pointed out! I'm always amazed by how quick you and @blegat are.

odow commented 7 months ago

No problem. A lot of the problems are because you might be the only person actively using JuMP for complex Hermitian stuff... 😆

araujoms commented 7 months ago

Well, one might say only, but I'd rather say first. :wink:

There are also these guys, but they invented their own reformulation of the complex Hermitian PSD cone, so they are only using JuMP for the real one.

odow commented 7 months ago

There are also these guys

You might recognize an author

araujoms commented 7 months ago

Well, I know some of them personally, but there is one that I'm sure you do. :laughing: