Refactor GeomMeanEpiCone into GenericConstraint #604
Conversation
|
I don't particularly like this design. The "Convex.jl" approach would probably be to add a |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
|
It's the concatenation that should belong to the cone so we should use a |
|
Now that #603 is fixed, I reverted the |
|
I had to refactor a bit in e220a8e to work around this bug of JuliaFormatter: domluna/JuliaFormatter.jl#833 |
src/constraints/GeomMeanEpiCone.jl
Outdated
|
|
||
| function GeomMeanEpiConeConstraint(T::Value, cone::GeomMeanEpiCone) | ||
| return GeomMeanEpiConeConstraint(constant(T), cone) | ||
| function Base.in(func::Tuple, set::GeomMeanEpiConeSquare) |
There was a problem hiding this comment.
We should probably get rid of this as it will become type piracy once GeomMeanEpiConeSquare is moved to MOI
There was a problem hiding this comment.
maybe we can have syntax like (A,B,C) in Constraint(MOI.GeomMeanEpiConeSquare()) instead, where Constraint would be owned by Convex
There was a problem hiding this comment.
Yes, this can be a follow up PR
There was a problem hiding this comment.
Let's just make this GenericConstraint(::Tuple, ::GeometricMeanEpiConeSquare) for now?
There was a problem hiding this comment.
Or Base.in(f::AbstractExpr, set::MOI.AbstractSet) = GenericConstraint(f, set)? Why do we need the tuple? Why not vcat(T, A, B)?
There was a problem hiding this comment.
It was just be able to throw the DimensionMismatch errors
There was a problem hiding this comment.
We can also just have some internal utility for now
There was a problem hiding this comment.
Like we did for _add_vectorized_exp_cone
There was a problem hiding this comment.
How about now? I've just gone for explicit Convex.GenericConstraint.
Co-authored-by: Benoît Legat <benoit.legat@gmail.com>
|
Looks good to me, any objection to merge ? |
This is an attempt to refactor
GeomMeanEpiConeintoGenericConstraint. In the long term (meaning definitely post v0.16), we can replace the custom_add_constraintwith 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 Constraintand renameGenericConstraintintoConstraint.This design is blocked by #603