Skip to content

Add vector version for setting constraint attributes #4033

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

Closed
wants to merge 1 commit into from

Conversation

blegat
Copy link
Member

@blegat blegat commented Jul 22, 2025

The user could also write the my_set_lower_bound of #4030 (comment)
However, calling this version is a bit easier to write but also a bit safer since it still calls check_belongs_to_model and takes care of the is_model_dirty flag.
One caveat is that users might then try doing it with JuMP containers so we may need at some point to also add support for AbstractArray that then collects and redirect to this method.
We could also check that the Vector have the same length here.

Closes #4030

Copy link

codecov bot commented Jul 22, 2025

Codecov Report

❌ Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.88%. Comparing base (b19c3e7) to head (67f3857).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/optimizer_interface.jl 0.00% 7 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##            master    #4033      +/-   ##
===========================================
- Coverage   100.00%   99.88%   -0.12%     
===========================================
  Files           43       43              
  Lines         6149     6156       +7     
===========================================
  Hits          6149     6149              
- Misses           0        7       +7     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@odow
Copy link
Member

odow commented Jul 24, 2025

I'm not convinced this is needed. It would still require users to understand the relationship between JuMP and MOI. The would need to know that set_lower_bound.(x, v) is slow, and that they really needed to update MOI.ConstraintSet for LowerBoundRef. So all this does it help then by calling backend(model) and index?

We already have too many methods in JuMP and MOI. I'm hesitant to add yet more.

@odow
Copy link
Member

odow commented Jul 24, 2025

More concretely:

using JuMP, HiGHS
N = 1_000
model = direct_model(HiGHS.Optimizer())
@variable(model, x[1:N] >= 0)
bounds = rand(N)
# Current option
MOI.set(
    backend(model),
    MOI.ConstraintSet(),
    index.(LowerBoundRef.(x)),
    MOI.GreaterThan.(v),
)
# This PR:
MOI.set(
    model,
    MOI.ConstraintSet(),
    LowerBoundRef.(x),
    MOI.GreaterThan.(v),
)

I don't really see that as a win.

I think I'd prefer we had fewer special cases and a more documented way to lower from JuMP to MOI.

@blegat
Copy link
Member Author

blegat commented Jul 24, 2025

Those two options are not equivalent since the first one does not check check_belongs_to_model nor set the dirty flag. This is the reason we cannot really advise the user to use the MOI level. Here, we're just adding a JuMP equivalent to an existing MOI method in order to allow the user have these checks.

@odow
Copy link
Member

odow commented Jul 24, 2025

check_belongs_to isn't really needed if the user is calling MOI. We assume that they know what they're doing.

Similarly with the dirty flag. The point of it was to catch differences in solvers. It someone reaches for backend( then all bets are off the table.

The current option also avoids an extra vector allocation since index.(LowerBoundRef.( will be fused into a single broadcast.

@odow
Copy link
Member

odow commented Jul 31, 2025

The more I think about this the less I like it.

@blegat
Copy link
Member Author

blegat commented Aug 1, 2025

Actually, one issue is that if we start on this road, we'll also probably need to support containers as wellw extracting the vectorization of the underlying constraints

@odow
Copy link
Member

odow commented Aug 1, 2025

Agreed. Close this then? People can use MOI if they really need to, and that means reshaping into a vector if they need.

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.

Vectorized set_lower_bound and set_upper_bound
2 participants