Refactor some constraint into GenericConstraint

[blegat]

This is a first step towards allowing to create constraints with custom MOI vector sets.

[odow]

Let me know when you'd like me to take a look

[odow]

Why would this be breaking for #589?

[blegat]

PositiveSemidefiniteConeConstraint etc... disappear and the sign of the dual of <= constraint is swapped

[odow]

PositiveSemidefiniteConeConstraint etc... disappear

We could keep the constant as an alias.

and the sign of the dual of <= constraint is swapped

This needs a much bigger discussion. We could change it without this PR, but it's horribly breaking because it silently swaps the sign.

I don't know how many people rely on this though.

Convex.jl/src/constraints/LessThanConstraint.jl

Lines 63 to 71 in 7eb1726

	function populate_dual!(model::MOI.ModelLike, c::LessThanConstraint, indices)
	# FIXME(odow): this dual is the 'wrong' sign, because Convex implements
	# rhs - lhs in Nonnegatives for a LessThanConstraint, instead of
	# lhs - rhs in Nonpositives.
	# Should we fix this?
	ret = MOI.get(model, MOI.ConstraintDual(), indices)
	c.dual = output(reshape(ret, c.size))
	return
	end

[blegat]

We could keep the constant as an alias.

Yes we could, but since this release is changing the name already, it's already breaking this so by removing it before even introducing it, it's not more breaking. Not having these aliases would simplify things, you can still write GenericConstraint{MOI...} which isn't too bad

This needs a much bigger discussion. We could change it without this PR, but it's horribly breaking because it silently swaps the sign.

We can also avoid breaking this by using GenericConstraint{MOI.Nonnegatives} for both cases. It's just seem kind of weird that the sign isn't swapped when you swap the inequality so I see it more as a bug-fix (you seem to agree in view of the comment you wrote in the code).

[odow]

We can also avoid breaking this by using GenericConstraint{MOI.Nonnegatives} for both cases

👍 at least for this PR.

you seem to agree in view of the comment you wrote in the code

I agree that we should fix it. But I don't know how to do it in a way that doesn't trip people up. At the very least, it needs to be a single PR just for that change.

[ericphanson]

I think it would be good to give the expected sign, and I'm not sure so many folks are using the existing functionality; there's been no issues about the sign at least. Maybe we could tag a v0.15.5 with a warning in getproperty for the field dual that the sign is changing, so anyone accessing it will be warned? Or maybe the release notes are enough.

[blegat]

We are already changing the sign for the PSD cone:

Convex.jl/src/constraints/PositiveSemidefiniteConeConstraint.jl

Lines 82 to 84 in 7eb1726

	⪯(x::AbstractExpr, y::AbstractExpr) = ⪰(y, x)
	⪯(x::Value, y::AbstractExpr) = ⪰(y, x)
	⪯(x::AbstractExpr, y::Value) = ⪰(y, x)

so it's maybe consistent to consider that a <= b is b - a in Nonnegatives() as well. In JuMP, we avoid it by requiring the cone to be given explicitly for conic inequalities so rewriting @constraint(model, a <= b, PSDCone()) into b - a in PSDCone() shouldn't be confusing or surprising.

If we want to be on the safe side we could also simply throw a warning in v0.16 when someone gets a dual with a <= inequality and then in v0.17 we can add GenericConstraint{MOI.Nonpositives} constraints.

[odow]

I've changed the sign in #593

[odow]

I think we should consider releasing v0.16 without this PR.

There are a lot of good changes sitting on master, and this is a nice-to-have, but not a necessity. It's also not obvious how you could write things in a generic way that works for arbitrary sets. You'll probably need specialized code for each set anyway. So we could always add this as a feature addition for other sets in MOI, and only change things over in the 1.0 release.

[blegat]

So we could always add this as a feature addition for other sets in MOI, and only change things over in the 1.0 release.

I don't know how far we are from v1.0, I think this will be a v0.17 probably. Since v0.16 is going to be breaking, and v0.17 as well, it just think it might be best to break once to avoid people having to change back and forth.

It's also not obvious how you could write things in a generic way that works for arbitrary sets.

Not much need to be done it seems. I'm planning to add default fallbacks in a follow-up PR. head could just print the set type, is_feasible could use MathOptSetDistances, set_with_size could use MOI.Utilities.set_with_dimension if the size is (n, 1) and error if there is more than 1 column. So the only custom method that would be needed for every set is vexity.

[odow]

Not much need to be done it seems.

There are still lots of test errors?

[odow]

Okay, I've added Zeros and SecondOrderCone support.

I've left the existing ExponentialConeConstraint, GeomMeanEpiCone, GeomMeanHypoCone, and RelativeEntropyEpiCone connstraints alone because they do something other than a trivial MOI constraint. (ExponentialConeConstraint in particular is a broadcast over arrays, rather than a single MOI.ExponentialCone constraint.)

[blegat]

There are still lots of test errors?

I meant that not much need to be done "per set" once we have GenericConstraint working