Conversation
Using evaluate_expression() with constant=True. This raises exceptions that reflect the key error messages that we want to return. Altogether, this avoids evaluating an expression tree twice.
Codecov Report
@@ Coverage Diff @@
## master #467 +/- ##
=========================================
+ Coverage 64.99% 65% +0.01%
=========================================
Files 357 357
Lines 59681 59699 +18
=========================================
+ Hits 38787 38806 +19
+ Misses 20894 20893 -1
Continue to review full report at Codecov.
|
jsiirola
left a comment
There was a problem hiding this comment.
It took me a couple passes to figure out what you are doing here and why (I can't imagine how you came across this).
Overall the change makes sense. All of my comments are Editorial, but there are enough that i would like to see a quick cleanup commit before we merge. I am happy to do that if you need.
|
|
||
|
|
||
| if node.is_variable_type(): | ||
| if node.fixed: |
There was a problem hiding this comment.
Why is this using an attribute instead of the is_fixed method?
| # Disable all logging while evaluating the expression | ||
| # | ||
| #logging.disable(logging.CRITICAL) | ||
| try: |
There was a problem hiding this comment.
This comment and the associated commented code should be removed.
pyomo/core/base/indexed_component.py
Outdated
| value() function.""" % ( self.name, i )) | ||
|
|
||
| except ValueError: | ||
| # |
There was a problem hiding this comment.
This except clause doesn't do anything and should be removed.
pyomo/core/base/indexed_component.py
Outdated
| raise | ||
|
|
||
| #finally: | ||
| # logging.disable(logging.NOTSET) |
There was a problem hiding this comment.
Commented out code should be removed.
CHANGELOG.txt
Outdated
| - Adding TerminationCondition and SolverStatus to pyomo.environ (#429) | ||
| - Resolved problems with ExternalFunctions with fixed arguments (#442) | ||
| - Adding the Pyomo5 expression system, which supports PyPy (#272) | ||
| - Reworking unhashable indexing (#467) |
There was a problem hiding this comment.
This is editorial, but we didn't rework indexing. Could you make it "Efficiency improvements when processing unhashable component indices"?
| return False, None | ||
|
|
||
|
|
||
| def evaluate_expression(exp, exception=True, constant=False): |
There was a problem hiding this comment.
Docstring needs "constant" added to it.
|
@jsiirola Sure. Go ahead and make your changes. I'd rather work collaboratively on PRs. |
Renaming the `_ConstantEvaluationVisitor` to `_EvaluateConstantExpressionVisitor` and adding documentation for the `constant=` argument to `evaluate_expression()`
Fixes N/A.
Summary/Motivation:
Eliminate duplicate walking of the expression tree when processing unhashable index values.
Changes proposed in this PR:
This PR creates an extension to evaluate_expression() with the 'constant' argument. When constant=True, this raises exceptions that reflect the key error messages that we want to return: (a) variables and (b) mutable parameters.
This change doesn't directly impact Pyomo performance in most cases, but the following script illustrates a case where the new code is twice as fast as the master branch:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: