search

jump-dev / Convex.jl

A Julia package for disciplined convex programming
https://jump.dev/Convex.jl/stable/
Other
559 stars 119 forks source link

Refactor GeomMeanEpiCone into GenericConstraint #604

Closed blegat closed 2 months ago

blegat commented 3 months ago

This is an attempt to refactor GeomMeanEpiCone into GenericConstraint. In the long term (meaning definitely post v0.16), we can replace the custom _add_constraint with a bridge so that the cone can be used in JuMP as well. If we do the same for the two other remaining constraints, we can then get rid of abstract type Constraint and rename GenericConstraint into Constraint. This design is blocked by https://github.com/jump-dev/Convex.jl/issues/603

odow commented 2 months ago

I don't particularly like this design. The "Convex.jl" approach would probably be to add a TupleAtom?

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 97.87234% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 97.84%. Comparing base (bb17c4c) to head (02c96ab). Report is 3 commits behind head on master.

Files Patch % Lines
src/expressions.jl 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #604 +/- ## ========================================== + Coverage 97.82% 97.84% +0.02% ========================================== Files 88 88 Lines 5140 5112 -28 ========================================== - Hits 5028 5002 -26 + Misses 112 110 -2 ```

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

blegat commented 2 months ago

It's the concatenation that should belong to the cone so we should use a vcat atom (which is seems gets rewritten with hcat and reshapes). If getindex on a vcat atom, would check if the index fit exactly in one of the argument of vcat and then in that case unwrap the vcat atom then it would fix https://github.com/jump-dev/Convex.jl/issues/603

blegat commented 2 months ago

Now that https://github.com/jump-dev/Convex.jl/issues/603 is fixed, I reverted the Tuple approach

blegat commented 2 months ago

I had to refactor a bit in e220a8e to work around this bug of JuliaFormatter: https://github.com/domluna/JuliaFormatter.jl/issues/833

blegat commented 2 months ago

Looks good to me, any objection to merge ?