Add output_symbols arg to codegen as performance mitigation
#361
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| # Convert to Matrix before calling free_symbols because it's much faster to call once | ||
| return sf.S(sf.Matrix(codegen_util.flat_symbols_from_values(self.outputs)).mat).free_symbols | ||
| @staticmethod | ||
| def _compute_output_symbols(output_values: Values) -> T.Set[sf.Symbol]: |
There was a problem hiding this comment.
Prefer to add back the comments stating that this is slow.
| @@ -159,6 +164,12 @@ def __init__( | |||
| self.inputs = inputs | |||
| self.outputs = outputs | |||
|
|
|||
| if output_symbols is None: | |||
| # compute the output symbols from the expressions: | |||
There was a problem hiding this comment.
Why do you end sentences in ":"? 🦀
| @@ -828,6 +838,7 @@ def with_jacobians( | |||
| include_results: bool = True, | |||
| name: str = None, | |||
| sparse_jacobians: bool = False, | |||
| output_symbols: T.Optional[T.Set[sf.Symbol]] = None, | |||
There was a problem hiding this comment.
I think this one still needs to be forwarded to the Codegen constructor below?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For complex expressions, the calculation of
Codegen.output_symbolscan grow quite large (encroaching on a minute or more). In some cases, the value ofoutput_symbolscan be specified externally. For example:output_symbolsreadily available at the callsite (frequently it matches the input symbol set exactly).output_symbolscan be determined from anotherCodegenobject, for example in most (though not all) cases the following can work:Ideally
free_symbolswould be faster, but this is a mitigation/workaround that can offer worthwhile savings in some cases.This change adds an optional argument to the various constructors of
Codegenso that the user can optionally provide the symbols themselves, and avoid an expensive computation.