Extend fbbt.ExpressionBoundsVisitor to handle relational expressions and Expr_if
#3129
Conversation
| def __init__(self, val): | ||
| self._val = val | ||
|
|
||
| def __bool__(self): |
There was a problem hiding this comment.
Do we actually want to implement __bool__? I feel like a value property or method would be safer. Maybe I'm being paranoid.
There was a problem hiding this comment.
I think so? - it made some of the logic in the walker easier, and the object will never be released anywhere the user would see it.
| ans = Bool(False), Bool(True) | ||
| else: | ||
| super().unexpected_expression_type(visitor, node, *args) | ||
| logger.warning( |
There was a problem hiding this comment.
Will this issue a warning for external functions? If so, I think that needs changed.
There was a problem hiding this comment.
It will not: ExternalFunctions are a) caught in the beforeChild callback (so exitNode should never be called, and b) there is an explicit registration for ExternalFunctionExpression, so it will never hit the unexpected_xpression_type method.
pyomo/contrib/fbbt/interval.py
Outdated
| def Bool(val): | ||
| return _true if val else _false |
There was a problem hiding this comment.
Should this (and maybe the bool_ class too) be private?
There was a problem hiding this comment.
Bool (now BoolFlag) should probably not be private, as it is used outside of this module (however, it should definitely not be added to __init__.py). bool_ has been renamed _bool_flag)
| self.assertEqual(len(end), 8) | ||
| self.assertEqual(len(end), 9) |
There was a problem hiding this comment.
What is this testing? Why did it change?
There was a problem hiding this comment.
This changed because the ExitNodeDispatcher changed a bit: by getting rid of the extra registration function (and implementing it's logic in __missing__, we changed how we handle unknown types: they are now registered witht he dispatcher using the unexpected_expression_type handler (so that handler can be overwritten). The test now reflects that the number of dispatchers registered in the END will increase.
Fixes #3083 .
Summary/Motivation:
This is an extended implementation of #3086 that switches the handling of unrecognized types from generating exceptions to emitting warnings. It also fixes #3083 by adding support for relational expressions and
Expr_if.I think it would be worth a future PR to revisit the design of the BeforeChildDispatcher to simplify the amount of custom code required between the LinearRepnVisitor / AMPLRepnVisitor and the ExpressionBoundsVisitor.
Changes proposed in this PR:
ExpressionBoundsVisitorto support relational andExpr_ifexpression sExpressionBoundsVisitorshould not raise exceptions for unrecognized numeric / logical expressions (instead just emit a warning)Legal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: