jump-dev / JuMP.jl

Modeling language for Mathematical Optimization (linear, mixed-integer, conic, semidefinite, nonlinear)
http://jump.dev/JuMP.jl/
Other
2.17k stars 390 forks source link

Fix error message if NonlinearExpression is mixed with new NL API #3741

Closed odow closed 1 month ago

odow commented 2 months ago

Closes #3740

TODO

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.37%. Comparing base (2693f57) to head (854e7d0). Report is 1 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3741 +/- ## ========================================== - Coverage 98.40% 98.37% -0.04% ========================================== Files 43 43 Lines 5820 5835 +15 ========================================== + Hits 5727 5740 +13 - Misses 93 95 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

pulsipher commented 2 months ago

This PR addresses mixing NonlinearExpressions with GenericNonlinearExpr, but we still get a MethodError if a NonlinearExpression is passed directly to @objective or @constraint:

using JuMP
m = Model()
@variable(m, x)
@NLexpression(m, nl_ex, x^3)
@objective(m, Min, nl_ex)
ERROR: MethodError: no method matching check_belongs_to_model(::NonlinearExpression, ::Model)

Closest candidates are:
  check_belongs_to_model(::ConstraintRef, ::AbstractModel)
   @ JuMP ~\.julia\packages\JuMP\027Gt\src\constraints.jl:63
  check_belongs_to_model(::ScalarConstraint, ::Any)
   @ JuMP ~\.julia\packages\JuMP\027Gt\src\constraints.jl:614
  check_belongs_to_model(::VectorConstraint, ::Any)
   @ JuMP ~\.julia\packages\JuMP\027Gt\src\constraints.jl:674
  ...

Stacktrace:
 [1] set_objective_function(model::Model, func::NonlinearExpression)
   @ JuMP ~\.julia\packages\JuMP\027Gt\src\objective.jl:129
 [2] set_objective(model::Model, sense::MathOptInterface.OptimizationSense, func::NonlinearExpression)
   @ JuMP ~\.julia\packages\JuMP\027Gt\src\objective.jl:176
 [3] macro expansion
   @ ~\.julia\packages\JuMP\027Gt\src\macros\@objective.jl:70 [inlined]
 [4] top-level scope
   @ REPL[5]:1

This can be remedied by adding something like:

function check_belongs_to_model(::NonlinearExpression, ::GenericModel)
   error("Expressions made with `@NLexpression` are only compatible for use with `@NLobjective` and `@NLconstraint`")
end
odow commented 2 months ago

if a NonlinearExpression is passed directly to @objective or @constraint:

Hahah but whhyyyyy would you do this. I'll fix :smile:

pulsipher commented 2 months ago

if a NonlinearExpression is passed directly to @objective or @constraint:

Hahah but whhyyyyy would you do this. I'll fix 😄

Never underestimate the power of typos and/or ignorance 😅

odow commented 2 months ago

Okay. I've actually tweaked things slightly, because this has nothing to do with the new API, it would also have been a bug in older versions of JuMP.

We now continue to play laissez-faire with expression construction--for better and worse--but we now error when we attempt to convert them to MOI functions. This provides a much more uniform error message for all the possible ways that they could end up being passed to the solver, and we can provide a nice error message.

pulsipher commented 1 month ago

I understand the logic behind relying on moi_function to provide the appropriate errors, but it still seems quite strange to me to have tests that check that mixing legacy NL objects with GenericNonlinearExpr works: https://github.com/jump-dev/JuMP.jl/blob/fbe96e521bc5ecaacb1e59f57f97df34626f2d4b/test/test_nlp_expr.jl#L374-L438 Why would this be expected behavior that we have tests that ensure it remains? What is the purpose of these tests?

odow commented 1 month ago

Okay, how about now.