Closed blnicho closed 1 year ago
I agree that the is_relational check probably needs to go into the writers either way. However, why not just disallow storing relational expressions in the Expression object as well? It seems to be a common mistake that users try to do:
m.e = Expression(expr= m.x <= 1)
m.c = Constraint(expr= m.e)
@ghackebeil I completely agree, in fact I always assumed that Expressions could not hold relational expressions until today when @jsiirola told me otherwise. Does anyone have any strong use-cases for allowing relational expressions in Expression components?
In fact, even leaving Expression out of this, we still don't check if a relational expression was passed to Objective. The following is sent to a solver with no error:
from pyomo.environ import *
m = ConcreteModel()
m.v = Var()
m.obj = Objective(expr=m.v**2>=10)
It looks like a check for is_relational is missing just for the nl writer, for the linear case an error is thrown in generate_canonical_repn
.
That's a good catch. I believe Objective is a subclass of Expression though, so disallowing it there would also cover this second case.
Since this works with the NL writer (as in, there is a legitimate way to express this using symbols recognized by the Ampl Solver Library, and we do that), I guess we have to ask ourselves if there are any solvers / users making use of this. If so, we might need to leave it in.
I am not in favor of disallowing relational (logical) expressions in Expression components. One of the things we would like to do is add explicit support for logical expressions (first for GDP, then to support Constraint Programming modeling approaches). In those contexts I could imagine putting a logical expression in an Expression would be useful.
@jsiirola: Are you in favor of at least temporarily disallowing it until someone actually adds this kind of functionality? I imagine the difference will be 1-2 lines of code in the Expression file.
@blnicho @jsiirola @ghackebeil - I'm going through old bug reports. Is this bug still present in the most recent Pyomo version?
@mrmundt Yes, this still exists, and I now disagree with my comment from 4 years ago (that was before we finished thinking through / implementing the LogicalExpression system). We should definitely disallow relational expressions in Expression
components. This is actually part of a larger issue that relational expressions should be moved out of the Numeric expression hierarchy and into the Logical expression hierarchy.
The following model is sent to a solver without any warning or error:
While the user should be able to construct this model, the writers should be checking
is_relational
on the Expression object before the model is sent to a solver.