Fix error message if NonlinearExpression is mixed with new NL API

[odow]

Closes #3740

TODO

• https://github.com/jump-dev/JuMP.jl/actions/runs/8914238919

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@            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     

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

[pulsipher]

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]

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

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

[pulsipher]

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]

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]

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:

JuMP.jl/test/test_nlp_expr.jl

Lines 374 to 438 in fbe96e5

	function test_nlparameter_interaction()
	model = Model()
	@variable(model, x)
	@NLparameter(model, p == 1)
	e = x + p
	@test e isa GenericNonlinearExpr
	@test string(e) == "x + ($p)"
	return
	end
	function test_nlexpression_interaction()
	model = Model()
	@variable(model, x)
	@NLexpression(model, expr, sin(x))
	e = x + expr
	@test e isa GenericNonlinearExpr
	@test string(e) == "x + ($expr)"
	return
	end
	function test_nlobjective_with_nlexpr()
	model = Model()
	@variable(model, x)
	y = sin(x)
	@NLobjective(model, Min, y^2)
	nlp = nonlinear_model(model)
	@test isequal_canonical(jump_function(model, nlp.objective), sin(x)^2)
	return
	end
	function test_nlconstraint_with_nlexpr()
	model = Model()
	@variable(model, x)
	y = sin(x)
	@NLconstraint(model, c, y^2 <= 1)
	nlp = nonlinear_model(model)
	@test isequal_canonical(
	jump_function(model, nlp.constraints[index(c)].expression),
	sin(x)^2 - 1,
	)
	return
	end
	function test_jump_function_nonlinearexpr()
	model = Model()
	@variable(model, x)
	@NLparameter(model, p == 1)
	@NLexpression(model, expr1, sin(p + x))
	@NLexpression(model, expr2, sin(expr1))
	nlp = nonlinear_model(model)
	@test string(jump_function(model, nlp[index(expr1)])) == "sin(($p) + $x)"
	@test string(jump_function(model, nlp[index(expr2)])) == "sin($expr1)"
	return
	end
	function test_constraint_object()
	model = Model()
	@variable(model, x)
	y = sin(x)
	@NLconstraint(model, c, y^2 <= 1)
	con = constraint_object(c)
	@test isequal_canonical(con.func, sin(x)^2 - 1.0)
	@test con.set == MOI.LessThan(0.0)
	return
	end

Why would this be expected behavior that we have tests that ensure it remains? What is the purpose of these tests?

[odow]

Okay, how about now.

[pulsipher]

Looks good to me, thanks!