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

Update list of excluded nonlinear tests #587

Closed
wants to merge 4 commits into from
Closed

Update list of excluded nonlinear tests #587

wants to merge 4 commits into from

Conversation

odow
Copy link
Member

@odow odow commented Nov 13, 2024

Let's see what the damage is

Copy link

codecov bot commented Nov 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.61%. Comparing base (729ecb9) to head (5815475).

❗ There is a different number of reports uploaded between BASE (729ecb9) and HEAD (5815475). Click for more details.

HEAD has 9 uploads less than BASE
Flag BASE (729ecb9) HEAD (5815475)
19 10
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #587      +/-   ##
==========================================
- Coverage   92.16%   86.61%   -5.56%     
==========================================
  Files           6        6              
  Lines        2630     2630              
==========================================
- Hits         2424     2278     -146     
- Misses        206      352     +146     

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

@odow
Copy link
Member Author

odow commented Nov 13, 2024

These are a blocker for release. I need to look into what's going on. There might be an issue with bridging the scalar nonlinear objective function.

@odow
Copy link
Member Author

odow commented Nov 13, 2024

Ah. I think it might be

vi, ci = MOI.add_constrained_variable(model, s)

which mucks up everything because the user didn't add these variables.

@simonbowly
Copy link
Collaborator

The first issue I run into if I enable only test_nonlinear_expression_hs071 is that the test seems to get locked in an infinite loop. But this test should not run at all since:

MOI.supports(model, MOI.ObjectiveFunction{F}())

is false for Gurobi.jl. Further if I just run the body of the test:

using Test
using Gurobi

MOI = Gurobi.MOI

env = Gurobi.Env()
model = Gurobi.Optimizer(env)

MOI.Utilities.loadfromstring!(
    model,
    """
    variables: w, x, y, z
    minobjective: ScalarNonlinearFunction(w * z * (w + x + y) + y)
    c2: ScalarNonlinearFunction(w * x * y * z) >= 25.0
    c3: ScalarNonlinearFunction(w^2 + x^2 + y^2 + z^2) == 40.0
    w in Interval(1.0, 5.0)
    x in Interval(1.0, 5.0)
    y in Interval(1.0, 5.0)
    z in Interval(1.0, 5.0)
    """,
)

then I get the expected error

Attribute MathOptInterface.ObjectiveFunction{MathOptInterface.ScalarNonlinearFunction}() is not supported by the model.

So is there a testing issue to sort out first? Or is MOI confused by the support for nonlinear constraints but not nonlinear objectives?

@torressa
Copy link
Collaborator

torressa commented Nov 13, 2024

So is there a testing issue to sort out first? Or is MOI confused by the support for nonlinear constraints but not nonlinear objectives?

The @requires MOI macro works when running in isolation but not when running the test suite 🤷
It errors when checking the objective function and doesn't proceed

julia> @requires MOI.supports(model, MOI.ObjectiveFunction{F}())
ERROR: LoadError: RequirementUnmet: MOI.supports(model, MOI.ObjectiveFunction{F}())

@simonbowly
Copy link
Collaborator

Ah. I think it might be

vi, ci = MOI.add_constrained_variable(model, s)

which mucks up everything because the user didn't add these variables.

Your point is for e.g. the hs071 tests, the following does not work as expected due to the added variables?

    x = MOI.get(model, MOI.ListOfVariableIndices())
    MOI.set.(model, MOI.VariablePrimalStart(), x, [1.0, 5.0, 5.0, 1.0])

So we should be adding the variables directly in the Gurobi API, not through MOI?

@odow
Copy link
Member Author

odow commented Nov 13, 2024

The tests run with a bridge that reformulates the objective into an epigraph constraint:

model =
MOI.Bridges.full_bridge_optimizer(Gurobi.Optimizer(GRB_ENV), Float64)

So we should be adding the variables directly in the Gurobi API, not through MOI?

Yes, we also need to check that any other bookkeeping related to adding variables/columns is still valid.

@odow
Copy link
Member Author

odow commented Nov 14, 2024

The tests are still running locally, but this is going in the right direction.

@simonbowly
Copy link
Collaborator

@odow do you need any input from us here? Are the additional variables something that JuMP/MOI can handle, or not?

@odow odow closed this Nov 25, 2024
@odow odow reopened this Nov 25, 2024
@odow
Copy link
Member Author

odow commented Nov 25, 2024

The additional variables need to be filtered out of all the various calls so that from MOI's point of view they don't exist.

This also affects variable deletion, etc.

I think the current master works for basic build-and-solve examples, but you can probably easily break if you start adding and deleting variables and constraints between solves.

This PR is a good start if you want to keep going. I don't remember where I got up to exactly, but I think it's a blocker for releasing v12, unless we want to mark it as experimental with some known bugs.

@simonbowly
Copy link
Collaborator

Why add them to the MOI layer if they just need to be filtered out at every step afterwards? Wouldn't it be simpler not to track them at all in MOI and have them only exist in the Gurobi model?

@odow
Copy link
Member Author

odow commented Nov 25, 2024

I think I tried that to begin with, but then it made deletion hard. If you delete the nonlinear constraint you also have to delete the variable and then shift all the indices?

This PR might not be the best approach.

@simonbowly
Copy link
Collaborator

I think I tried that to begin with, but then it made deletion hard. If you delete the nonlinear constraint you also have to delete the variable and then shift all the indices?

True, but @torressa sorted out that logic for deletion of regular variables, so I imagine the same approach would work here. I'll give it a go (in a separate PR to avoid breaking your work here).

@odow
Copy link
Member Author

odow commented Nov 27, 2024

Closing in favour of #590

@odow odow closed this Nov 27, 2024
@odow odow deleted the odow-patch-1 branch November 29, 2024 20:32
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.

3 participants