Refactor linear/quadratic expression compilers #3651
Conversation
… invalid argument
- fix BeforeChildDispatcher inheritance - leverage constant_flag / multiplier_flag for checking coefficients
…ize var_order dict
- changing argument ordering - more consistent handling of 0*expressions and quadratic results
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3651 +/- ##
==========================================
- Coverage 88.93% 88.91% -0.02%
==========================================
Files 889 888 -1
Lines 102486 103195 +709
==========================================
+ Hits 91144 91756 +612
- Misses 11342 11439 +97
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| return val | ||
| return 2 # something not 0 or 1 | ||
|
|
||
| @staticmethod | ||
| def multiplier_flag(val): | ||
| if val.__class__ in native_numeric_types: | ||
| if not val: | ||
| return 2 | ||
| return val | ||
| return 2 # something not 0 or 1 |
There was a problem hiding this comment.
Should these both return 2?
There was a problem hiding this comment.
Sure. It really doesn't matter. The point (as indicated by the comment) is that it is just anything other 0 or 1. Because "2" is not unique (the val could actually be float(2)), there really isn't a driver to differentiate between 0, 2, and non-native numeric types.
pyomo/repn/quadratic.py
Outdated
| if not other_mult_flag: | ||
| # 0 * other, so you would think that there is nothing to | ||
| # add/change about self. However, there is a chance | ||
| # that other contains an InvalidNumber, so we should go | ||
| # looking for it... | ||
| if other.constant != other.constant: | ||
| self.constant += mult * other.constant | ||
| for vid, coef in other.linear.items(): | ||
| if coef != coef: | ||
| if vid in self.linear: | ||
| self.linear[vid] += mult * coef | ||
| else: | ||
| self.linear[vid] = mult * coef | ||
| if other.quadratic: | ||
| for vid, coef in other.quadratic.items(): | ||
| if coef != coef: | ||
| if not self.quadratic: | ||
| self.quadratic = {} | ||
| if vid in self.quadratic: | ||
| self.quadratic[vid] += mult * coef | ||
| else: | ||
| self.quadratic[vid] = mult * coef | ||
| else: |
There was a problem hiding this comment.
There are a lot of missed lines here w.r.t. tests. Might be worth adding at least one test.
| self.assertIsNone(repn.quadratic) | ||
| self.assertEqual(repn.quadratic, None) |
There was a problem hiding this comment.
Why not just use assertIsNone?
There was a problem hiding this comment.
This was part of leftover debugging. The error you get when repn.quadratic is not None is remarkable unhelpful, whereas using assertEqual gives an error message that says what repn.quadratic actually is.
| @@ -955,7 +995,7 @@ def test_factor_multiplier_simplify_coefficients(self): | |||
| self.assertEqual(cfg.var_order, {id(m.x): 0, id(m.z): 1}) | |||
| self.assertEqual(repn.multiplier, 1) | |||
| self.assertIsNone(repn.nonlinear) | |||
| self.assertEqual(repn.quadratic, {}) | |||
| self.assertEqual(repn.quadratic, None) | |||
There was a problem hiding this comment.
Why not just use assertIsNone?
| @@ -936,7 +976,7 @@ def test_finalize_simplify_coefficients(self): | |||
| self.assertEqual(repn.multiplier, 1) | |||
| assertExpressionsEqual(self, repn.constant, 2 * m.y**2) | |||
| self.assertEqual(repn.linear, {id(m.z): -1}) | |||
| self.assertEqual(repn.quadratic, {}) | |||
| self.assertEqual(repn.quadratic, None) | |||
There was a problem hiding this comment.
Why not just use assertIsNone?
Fixes # .
Summary/Motivation:
The linear / quadratic / parameterized / templated expression walkers and compilers contain a large amount of repeated code. This PR refactors the walkers so that the bulk of repeated code can be deleted. The key changes are:
Other changes in this PR
@disable_methodsdecorator). This follows the pattern used elsewhere (e.g., in Constriant)While this PR will slow down the LP writer a little (1-2%), it resolves enough issues in the Parameterized and Templatized walkers that I think we should adopt it. There are a number of future developments in the works (that build on this PR) that should more than make up for this performance hit [moving the LP writer to leverage the standard form compiler to eliminate the need for sorting in the lp_writer and the use of dicts in the compiler for coefficient deduplication).
Changes proposed in this PR:
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: