Bugfixes and testing of linear templates#3781
Conversation
- iterate over the underlying reference when guessing the dimen This prevents recursion when templatizing. - inconsistent dimen is reported as None
This reverts commit c7c5d6f.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3781 +/- ##
==========================================
+ Coverage 89.92% 90.03% +0.10%
==========================================
Files 901 902 +1
Lines 106285 106496 +211
==========================================
+ Hits 95580 95882 +302
+ Misses 10705 10614 -91
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:
|
emma58
left a comment
There was a problem hiding this comment.
I kind of ran out of brainpower in the crux, so I'll look at linear_template.py again in the morning, but here are a few comments from elsewhere. I'm very exited that it's tested!!
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def _validate_generator(generator): |
There was a problem hiding this comment.
Naming question: Should this be _check_generator_templatizable or something of the like? It's not really invalid... Just impossible.
There was a problem hiding this comment.
I don't know. Naming is hard. This isn't checking that the generator is templatizable - that has already happened (when we call next(generator) in sum_template and don't get a TemplateExpressionError). So at this point, we already have a template expression and we are looking into the state of the generator to check that the internal state of the generator is "valid" (i.e., it doesn't have any local variables that are not IndexTemplate objects). So we really are validating that the generator is (in a) "correct" (state).
There was a problem hiding this comment.
Ah, okay. Then maybe your name makes sense!
| if type(d) is not int: | ||
| # This covers None (jagged set) and UnknownSetDimen. In | ||
| # both cases, we will not attempt to unpack the Set and just | ||
| # assume a single index template. |
There was a problem hiding this comment.
Why is this safe for UnknownSetDimen? Or actually either of them?
There was a problem hiding this comment.
Because will it catch if someone is trying to templatize an expression where they unpacked something multidimensional? Or is the idea that that will die then, so just keep going for now?
There was a problem hiding this comment.
What is happening here is that we don't know how many scalars (i.e., IndexTemplates) to unpack the set item into. So we won't try and unpack anything (and hope that the user won't either). In this case we will return a single IndexTemplate that is the placeholder for the arbitrary tuple that would be returned by the Set. As long as the user doesn't try and unpack that tuple (and they shouldn't without first looking at the length of the tuple - which will generate the TemplateExpressionError that disables templatization).
Here's an example:
m.I = Set(dimen=None, initialize=[(1,), (2, 3), (4, 5, 6)])
m.x = Var(m.I)
m.c = Constraint(expr=sum(m.x[i] for i in m.I) <= 0)Here, i is a tuple of some "random" length. So we want to templatize this as
sum(x[_1] for _1 in I)
| # it is not clear what to import to get to the built-in "generator" | ||
| # type. We will just create a generator and query its __class__ | ||
| generator_validators = { | ||
| (_ for _ in ()).__class__: _validate_generator, |
There was a problem hiding this comment.
Wow, that one took me a minute, even with the comment! How strange that it's hard to import...
| niters = -len(self.cache) | ||
| expr = next(generator) | ||
| final_cache = len(self.cache) | ||
| return TemplateSumExpression((expr,), self.npop_cache(final_cache - init_cache)) | ||
| niters += len(self.cache) | ||
| if niters: |
There was a problem hiding this comment.
This is just me not understanding the old code either, but how does next(generator) change the state of self.cache?
There was a problem hiding this comment.
This is the core trick in Templatization of sum expressions: next(generator) calls the generator within the sum to return the first term. That in turn calls iter(...) on the thing (usually a Set) being summed over. For Set objects, that hits the overriden __iter__ implementation (see _set_iterator_template_generator, managed by _template_iter_context) that adds to the context cache)
There was a problem hiding this comment.
Ohhhhh. This would be a nice thing to write down in developer docs someday! :)
emma58
left a comment
There was a problem hiding this comment.
OK, please don't get hit by a bus, but I kind of understand this, and I reallllly like it!
| raise InvalidConstraintError( | ||
| "LinearTemplateRepn does not support constraints containing " | ||
| "general nonlinear terms." | ||
| ) |
There was a problem hiding this comment.
Is it possible to give the name or some pointer to the offending Constraint in this error?
Fixes # .
Summary/Motivation:
This PR is primarily adding more comprehensive testing of templatizing (and compiling) linear models. As part of flusing out the tests, this resolves a number of issues / bugs in the templatizer, the linear template compiler, and in general Pyomo:
General Pyomo changes
Param.extract_valuesto returndefaultdictfor Params with default valuesInvalidConstraintErrorexception for constraints that cannot be emitted to the solversSetOfwhen iterating over IndexedComponent objects indexed by non-finite SetsSetOf.dimenshould returnUnknownSetDimen(not 0) if the underlying reference has no dataUpdates to the templatizer
attempt_importto break circular dependencies intemplate_exprUnknownSetDimenwhen iterating over template generators and treat like jagged setssum()that do not sum over generatorssum()whose generator doesn't create a template indexsum()that appear to iterate over things other than Pyomo Set objectsUpdates to the linear template compiler
check_duplicatesandremove_fixed_varsoptions (this is not more efficiently implementable through sparse matrix operations0IndexedComponent.SkipChanges 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: