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

Improve matrix inequality support #3778

Merged
merged 5 commits into from
Aug 6, 2024
Merged

Improve matrix inequality support #3778

merged 5 commits into from
Aug 6, 2024

Conversation

odow
Copy link
Member

@odow odow commented Jun 25, 2024

Follow-up to #3766.

In our discussion of #3766, we never really talked about the fact that == is not ambiguous.

@odow odow changed the title Add symmetric matrix equality support Add Array equality support Jun 25, 2024
test/test_macros.jl Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jun 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.40%. Comparing base (95da051) to head (4228d97).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3778   +/-   ##
=======================================
  Coverage   98.40%   98.40%           
=======================================
  Files          44       44           
  Lines        5956     5966   +10     
=======================================
+ Hits         5861     5871   +10     
  Misses         95       95           

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

@odow odow changed the title Add Array equality support Improve matrix inequality support Jun 25, 2024
@blegat
Copy link
Member

blegat commented Jul 1, 2024

Can we just replace Nonnegatives here:

operator_to_set(::Function, ::Union{Val{:(>=)},Val{:(≥)}}) = Nonnegatives()

What are the other cases that you'd like to intercept ?

@odow
Copy link
Member Author

odow commented Jul 13, 2024

I didn't do this, because it could be interpreted as a breaking change.

@odow odow requested a review from blegat July 15, 2024 23:14
@odow
Copy link
Member Author

odow commented Jul 23, 2024

Bump @blegat

@blegat
Copy link
Member

blegat commented Jul 30, 2024

I didn't do this, because it could be interpreted as a breaking change.

Breaking change for which function ? We are changing it to _OpGreaterThan before giving it to build_constraint in this PR as well anyway.

@odow
Copy link
Member Author

odow commented Jul 30, 2024

Changes the return of operator_to_set, but perhaps this is minor: https://juliahub.com/ui/Search?type=code&q=operator_to_set

@blegat
Copy link
Member

blegat commented Jul 30, 2024

Indeed, the function could be used outside of a JuMP macro. I'd say it is minor. _intercept_operator is complicated or code too much just to accomplish to accomplish exactly the same thing as changing the returned value of operator_to_set

@odow
Copy link
Member Author

odow commented Jul 31, 2024

I made a slight tweak to get rid of _intercept_operator but add a new method to operator_to_set.

@blegat
Copy link
Member

blegat commented Jul 31, 2024

I still think this complicating things too much just for such a small breaking change. Users are expected to implement operator_to_set, we never said that calling it would always return the same thing. I can't believe we haven't made something that could qualify as breaking for the returned value of such function since JuMP v1.0

@odow
Copy link
Member Author

odow commented Aug 1, 2024

Okay how about now

src/macros/@constraint.jl Outdated Show resolved Hide resolved
src/macros/@constraint.jl Outdated Show resolved Hide resolved
@blegat
Copy link
Member

blegat commented Aug 1, 2024

I'd prefer to name things for what they are rather than what they are used for.
What about calling the set Nonnegative and saying that
@constraint(model, a - b in Nonnegative()) and @constraint(model, a >= b) are synonymous.

@odow
Copy link
Member Author

odow commented Aug 2, 2024

Let's please not have Nonnegative and Nonnegatives for the same but slightly different thing.

@blegat
Copy link
Member

blegat commented Aug 2, 2024

How about GreaterThanZero ?

@odow
Copy link
Member Author

odow commented Aug 4, 2024

This needs something similar to #3797

@odow odow changed the base branch from master to od/sym-dual August 5, 2024 02:42
@blegat
Copy link
Member

blegat commented Aug 5, 2024

This needs something similar to #3797

What do you mean ?

@odow
Copy link
Member Author

odow commented Aug 5, 2024 via email

Base automatically changed from od/sym-dual to master August 5, 2024 21:01
@odow
Copy link
Member Author

odow commented Aug 6, 2024

Okay, this is good to go on my end.

@odow odow merged commit 88738c4 into master Aug 6, 2024
11 checks passed
@odow odow deleted the od/equality branch August 6, 2024 06:56
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.

None yet

2 participants