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 set inequality syntax for matrices #3766

Merged
merged 10 commits into from
Jun 25, 2024
Merged

Add set inequality syntax for matrices #3766

merged 10 commits into from
Jun 25, 2024

Conversation

odow
Copy link
Member

@odow odow commented Jun 3, 2024

Copy link

codecov bot commented Jun 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.39%. Comparing base (b369f5b) to head (19c7f62).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3766   +/-   ##
=======================================
  Coverage   98.38%   98.39%           
=======================================
  Files          44       44           
  Lines        5885     5904   +19     
=======================================
+ Hits         5790     5809   +19     
  Misses         95       95           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

docs/src/manual/constraints.md Outdated Show resolved Hide resolved
@odow
Copy link
Member Author

odow commented Jun 4, 2024

I can see us getting asked to support this for all matrices, not just symmetric ones. But perhaps that' a PR for another day.

@LebedevRI
Copy link
Contributor

I can see us getting asked to support this for all matrices, not just symmetric ones. But perhaps that' a PR for another day.

FWIW, yes. The vec() escape hatch is meh, and may make issues w/ matrix shape mismatch (== comparing wrong things) less easy to discover.

@odow odow changed the title Add set inequality syntax for symmetric matrices Add set inequality syntax for matrices Jun 5, 2024
@blegat
Copy link
Member

blegat commented Jun 5, 2024

What we are doing for @constraint(model, x <= y, set) is a bit weird, it will give a value and dual that are inconsistent with the scalar @constraint(model, x <= y) (similar to what we noticed and fixed for Convex.jl). I'm wondering if, instead of interpreting it as y - x in set, we shouldn't rather see it as x - y in -set. We would still encode it as y - x in set but we could keep the -1 flip in the shape so that it's recovered when you query value or dual.

@blegat
Copy link
Member

blegat commented Jun 5, 2024

As this is breaking then maybe we should continue to do what we do but we should add a warning in the docs that @constraint(model, x >= y, set) is preferred since @constraint(model, x <= y, set) may have a behavior that's unexpected or confusing which makes the things less readable or error-prone if value or dual are used.

@LebedevRI
Copy link
Contributor

What we are doing for @constraint(model, x <= y, set) is a bit weird, it will give a value and dual that are inconsistent with the scalar @constraint(model, x <= y) (similar to what we noticed and fixed for Convex.jl).

I'm wondering if, instead of interpreting it as y - x in set,

Peanut gallery comment: the docs and disscussion in the issue make it seem
like x <= y is treated exactly as x >= y, i.e. it would have still resulted in x-y in set?

we shouldn't rather see it as x - y in -set. We would still encode it as y - x in set but we could keep the -1 flip in the shape so that it's recovered when you query value or dual.

This kinda sounds like either solution would address my concerns from #3765, i think.

As this is breaking then maybe we should continue to do what we do but we should add a warning in the docs that @constraint(model, x >= y, set) is preferred since @constraint(model, x <= y, set) may have a behavior that's unexpected or confusing which makes the things less readable or error-prone if value or dual are used.

Peanut gallery comment: the concern here is external non JuMP/MOI sets/code, correct? Since i don't think this syntax currently works on native JuMP/MOI stuff, so it would only affect extensions?

@odow
Copy link
Member Author

odow commented Jun 5, 2024

Let's just throw an error for these <= cases. I'll document the PSDCone case in a separate PR.

@blegat
Copy link
Member

blegat commented Jun 5, 2024

But it already does it for AbstractVector so wouldn't adding an error be breaking ?

@LebedevRI
Copy link
Contributor

LebedevRI commented Jun 5, 2024

Peanut gallery comment:
it's going to be unfortunate that there's difference in behavior in these inequality constraints.
From outside, what would //i// think the best, no-unexpected-behavior, be, is:

@constraint(model, x >= y)

is always the same as

@constraint(model, x - y >= 0)
@constraint(model, x - y in Nonnegatives)

likewise

@constraint(model, x <= y)

is always the same as

@constraint(model, x - y <= 0)
@constraint(model, x - y in Nonpositives)

... for any [common] type of x and y, be it either a scalar, a vector, a symmetric matrix, or a non-symmetric matrix.

Currently

@constraint(model, x >= y)
@constraint(model, x <= y)

are not valid, and the code is always expected to explicitly spell out the cone type.
They already do that, so that change would greatly improve the QoL for the "normal" comparisons,
make bugs basically impossible in them (have i mentioned that manually spelling the set is a bit mouthful?),
while not particularly affecting the cone constraints - one can't automatically guess
which constraint was meant there, wherein that is trivial and obvious in "normal" case...

As i see it, it's an obvious trade-off, and the current solution is not obviously optimal.
(If this makes any sense, but is clearly contentious, perhaps defer until the monthly(?) developer call?)

I'll shut up now.

@odow odow force-pushed the od/symmetric-inequality branch from e982863 to 7a9a6be Compare June 5, 2024 20:56
@odow
Copy link
Member Author

odow commented Jun 5, 2024

I've added an error for these new cases. No change to the existing vector support. I've added a warning to #3769, and we don't mention x <= y, Set() in the documentation.

@odow
Copy link
Member Author

odow commented Jun 6, 2024

I actually think this is less problematic that it seems. You're opting into a new syntax.

In Boyd's book, they have:

$x \preceq_K y \iff y - x \in K$

And they write

We also write $x \succeq_K y$ for $y \preceq_K x$

It's not obvious that flipping $\preceq$ to $\succeq$ should change the sign of the primal or dual value. That's just not the way you interpret it.

@LebedevRI
Copy link
Contributor

FWIW i believe this (& its non-symmetric friend) can proceed without waiting for the understanding in #3770 to be reached.

set_start_value.(y, [6 4; 4 7])
g = [x[1, 1] - y[1, 1], x[1, 2] - y[1, 2], x[2, 2] - y[2, 2]]
for set in (Nonnegatives(), Nonpositives(), Zeros())
c = @constraint(model, x >= y, set)
Copy link
Member Author

Choose a reason for hiding this comment

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

Add a test for x - y in set

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah. @mlubin, I remember now.

We can't support x - y in Nonnegatives() because then we can't tell it apart from @constraint(model, x >= y) which was the whole point in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

We can't support x - y in Nonnegatives()

What do you mean ? You mean x >= y, Nonnegatives() ?

Copy link
Member Author

Choose a reason for hiding this comment

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

No I mean @constraint(model, x - y in Nonnegatives()) because this hits the same codepath as @constraint(model, x >= y).

Copy link
Member Author

Choose a reason for hiding this comment

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

Unless I intercept the GreaterThan(false) method, but then I think the last time I looked that makes ambiguities.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that's the part I didn't get. It does make sense to intersect the GreaterThan then because having AbstractArray-in-GreaterThan is ambiguous and shouldn't be allowed while AbstractArray-in-Nonnegatives should be allowed.
So neither @constraint(model, x - y in GreaterThan(false)) nor @constraint(model, x >= y) will work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah. I forgot. We switched from GreaterThan(false) to Nonnegatives. So we can't even intercept. x >= y is exactly equal to x - y in Nonnegatives

Copy link
Member

Choose a reason for hiding this comment

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

Ouch, that was probably not ideal. Using Nonnegatives by default kind of means we interpret >= as elementwise by default.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could add a layer of indirection to fix that without being breaking

@odow
Copy link
Member Author

odow commented Jun 23, 2024

Let's merge this as a strict improvement for now. I can come back to the x - y in set case in a separate PR.

@odow
Copy link
Member Author

odow commented Jun 25, 2024

Merging this for now, but I'll hold off tagging a new release until I've had a go at x - y in Nonnegatives.

@odow odow merged commit 4db5a0c into master Jun 25, 2024
11 checks passed
@odow odow deleted the od/symmetric-inequality branch June 25, 2024 22:23
@odow odow mentioned this pull request Jun 25, 2024
1 task
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.

Inequality constraints over symmetric matrices are cumbersome
3 participants