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

Add Scaled{S<:AbstractSet} #2237

Merged
merged 3 commits into from
Sep 6, 2023
Merged

Add Scaled{S<:AbstractSet} #2237

merged 3 commits into from
Sep 6, 2023

Conversation

blegat
Copy link
Member

@blegat blegat commented Jul 5, 2023

This allows this vector to be used for other sets that have a non-standard set_dot.
Then, we could have

struct Scaled{S<:MOI.AbstractSet}
    set::S
end

and change the bridge into working for any S to Scaled{S}.
That way, https://github.com/blegat/Zaphod.jl, COSMO, SCS, ... can say that they only support Scaled{PSDTriangle}, Scaled{HermitianPSDTriangle}, Scaled{LogDetTriangle}, Scaled{RootDetTriangle}, ... and we don't have to have to add a scaled sets, scaled bridge, scaled scaling vector for each one. We just have to define set_dot and set_dual and the rest automatically works!

This PR takes code from Dualization added in jump-dev/Dualization.jl#135

@blegat blegat marked this pull request as ready for review August 7, 2023 12:38
@blegat blegat mentioned this pull request Aug 8, 2023
3 tasks
@odow
Copy link
Member

odow commented Aug 10, 2023

I think this needs a much deeper discussion before we add it. How many solvers would use this set? (And for what sets other than the existing scaled PSD cone?) What are the alternatives?

We already have far too many sets for matrices. Adding a Scaled{S} removes some duplication of code, but comes at the cost of a much greater surface area in what sets can be supported by the solver.

How would this look at the JuMP level? How would they write x in Scaled(PSD)? What are the elements of x (scaled or unscaled)?

If this is only going to be widely used by a single solver like Zaphod, then could it be added as a solver-specific set and bridge?

@blegat
Copy link
Member Author

blegat commented Aug 11, 2023

We already have far too many sets for matrices. Adding a Scaled{S} removes some duplication of code, but comes at the cost of a much greater surface area in what sets can be supported by the solver.

Yes, I think in MOI v2 we should only have XXX (with custom set_dot), Square{XXX} (with regular set_dot) and Scaled{XXX} (with regular set_dot) instead of having XXXTriangle, XXXSquare and ScaledXXX.
Then we'll have only 4 matrix sets: PSD, HermitianPSD, LogDetPSD and RootDetPSD which is much more reasonable.
We could also add Square{XXX} in MOI v1 and add backward compatibility but I won't push for this myself since I don't see it blocking at the moment.

How would this look at the JuMP level? How would they write x in Scaled(PSD)? What are the elements of x (scaled or unscaled)?

If you write that then x should be a vector since Scaled(PSD) is an MOI AbstractVectorSet and x should be the scaled version. No special case needed here.

If this is only going to be widely used by a single solver like Zaphod, then could it be added as a solver-specific set and bridge?

No, for the same reason many conic solver use ScaledPSDConeTriangle now that we have added it, they will use the scaled version of the other ones as well. Zaphod is meant to be a POC showing that conic solvers can be written using the MOI sets directly and tools that are built in in MOI (and MathOptSetDistances) instead of having to define internal sets as well and a mapping between them and the MOI ones (as done by COSMO since COSMO was written back in the time where it wasn't easy to do so). Once I add support for many sets in Zaphod, it can serve as POC for other ADMM-based solver to do the same (such as COSMO or @tjdiamandis https://github.com/tjdiamandis/GeNIOS.jl) and also for instance HermitianPSD to Clarabel since it's a symmetric cone

@odow
Copy link
Member

odow commented Aug 13, 2023

Yes, I think in MOI v2 we should only have XXX (with custom set_dot), Square{XXX} (with regular set_dot) and Scaled{XXX} (with regular set_dot)

I guess my point is: sure, but we currently have the suffix versions, so why complicate things now by adding yet more ways to write things down.

I don't see any solvers asking for ScaledLogDetCone, so this feels like churn for the sake of it, even if the final product would be nicer than what we already have.

I'm 👍 for adding this in MOI v2, but just not yet.

@blegat
Copy link
Member Author

blegat commented Aug 13, 2023

Let's not wait for MOI v2, we have user asking for HermitianPSD and logdet support natively online and to me directly.

@odow
Copy link
Member

odow commented Aug 14, 2023

we have user asking for HermitianPSD

They're asking for ScaledHermitianPSD explicitly, and this can't be achieved by just using HermitianPSD? Which solver? What application?

I'm not saying it isn't useful. I just don't know if the utility outweighs the cost of making this more complicated than it already is.

@odow odow changed the title Refactor symmetric matrix scaling vector Add Scaled{S<:AbstractSet} Aug 14, 2023
@blegat
Copy link
Member Author

blegat commented Aug 14, 2023

I'm not saying it isn't useful. I just don't know if the utility outweighs the cost of making this more complicated than it already is.

We basically need the scaled version for other cones for the same reason we need it for the PSD one. Conic solvers have moved to used MatrixOfConstraints so if they use the scaled version internally they will need to define their own set and bridges. Moreover, as I want to demonstrate in https://github.com/blegat/Zaphod.jl/, you could use MOI not only for the interface, but also internally in the solving process, e.g., the only thing you need is to implement a projection since it's implemented in MathOptSetDistances on MOI sets. I tried to make it work on non-scaled sets in blegat/Zaphod.jl#2 but my conclusion is that the homogeneous self-dual embedding is not self-dual if the sets does not have the canonical scalar product. Other solvers may need it for similar reasons.

@blegat blegat force-pushed the bl/set_dot_scaling branch 2 times, most recently from 89e243b to aae0b77 Compare August 14, 2023 08:13
@odow
Copy link
Member

odow commented Aug 14, 2023

My point is, concretely, which solvers are asking for this? We don't have any solvers natively supporting scaled Hermitian PSD cone or scaled LogDet. (Excluding Zaphod, but let's not add special features just for that.) The general rule has been not to add things unless multiple solvers support it.

@blegat
Copy link
Member Author

blegat commented Aug 14, 2023

Hypatia support scaled HermitianPSD natively and is currently scaling in the MOI wrapper to do the conversion.

The general rule has been not to add things unless multiple solvers support it.

We could wait for solvers to add support for it and have complicated MOI wrapper and then have to simplify them or add it in time so that they don't have to make it complicated. In quantum physics, you often need to solve SDPs with big hermitian matrices and having to transform to real PSD makes this a lot slower and create numerical issues. Now that we have HermitianPSD in MOI, conic solvers can add support for it but if their whole infrastructure assumes that the vector sets have the standard scalar product, the whole code will need to be more complicated to add that one set.

@blegat
Copy link
Member Author

blegat commented Sep 5, 2023

Unless any objection is raised, I'm going to merge this tomorrow to follow the decision from the jump-dev meeting and I'll follow with updating the wrappers in #2260

@odow
Copy link
Member

odow commented Sep 5, 2023

Let me take another look

src/Utilities/set_dot.jl Outdated Show resolved Hide resolved
src/Utilities/set_dot.jl Outdated Show resolved Hide resolved
src/Utilities/set_dot.jl Outdated Show resolved Hide resolved
Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

I'm still not sure we need this, but okay

@blegat blegat merged commit bff293a into master Sep 6, 2023
16 of 17 checks passed
@blegat blegat deleted the bl/set_dot_scaling branch September 6, 2023 13:52
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