New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add output_symbols
arg to codegen as performance mitigation
#361
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
# 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one still needs to be forwarded to the Codegen
constructor below?
For complex expressions, the calculation of
Codegen.output_symbols
can grow quite large (encroaching on a minute or more). In some cases, the value ofoutput_symbols
can be specified externally. For example:output_symbols
readily available at the callsite (frequently it matches the input symbol set exactly).output_symbols
can be determined from anotherCodegen
object, for example in most (though not all) cases the following can work:Ideally
free_symbols
would 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
Codegen
so that the user can optionally provide the symbols themselves, and avoid an expensive computation.