Ensure TemplateOptimization returns native-symbolic objects
#11107
Conversation
`TemplateOptimization` currently uses Sympy internally to get access to solvers that have no equivalent in Symengine. Previously, it then did not convert the solutions to its equations back into Symengine format (if appropriate) before passing them on to `ParameterExpression`, which could lead to different assumptions about the type of the contained objects and bugs if `ParameterExpression` attempted to use Symengine-specific forms of methods on Sympy objects.
jakelishman
added
Changelog: Bugfix
|
One or more of the the following people are requested to review this:
|
| # Check compatibility by solving the resulting equation | ||
| sym_sol = sym.solve(equations, set(temp_symbols.values())) | ||
| for key in sym_sol: | ||
| try: | ||
| sol[str(key)] = ParameterExpression(circ_dict, sym_sol[key]) | ||
| except TypeError: | ||
| return None | ||
|
|
||
| if not sol: | ||
| # Check compatibility by solving the resulting equation. `dict=True` (surprisingly) forces | ||
| # the output to always be a list, even if there's exactly one solution. | ||
| sym_sol = sym.solve(equations, set(temp_symbols.values()), dict=True) | ||
| if not sym_sol: | ||
| # No solutions. | ||
| return None | ||
|
|
||
| for key in temp_symbols: | ||
| fake_bind[key] = sol[str(key)] | ||
| # If there's multiple solutions, arbitrarily pick the first one. | ||
| sol = { | ||
| param.name: ParameterExpression(circ_dict, to_native_symbolic(expr)) | ||
| for param, expr in sym_sol[0].items() | ||
| } | ||
| fake_bind = {key: sol[key.name] for key in temp_symbols} |
There was a problem hiding this comment.
Technically this is a bit of a larger change than completely required, but I was having a hard time parsing what was going on with several objects having multiple possible types, being created long before being initialised, and try/except early returns.
Strictly there's a minor (positive) behavioural change here: previously, the try/except handling would cause matches that had multiple possible solutions to fail binding, whereas now it'll pick one of the solutions. This makes the pass strictly more powerful, since templates are isolated from each other based on maximal matching, but in practice, multiple solutions require non-linear parametrised circuits, and there aren't any of those in the template library.
There was a problem hiding this comment.
Does it make sense to doc the potential behavioural change in an upgrade release note?
There was a problem hiding this comment.
I think I prefer not adding a release note, because I'd prefer to not commit the API to supporting arbitrarily complex symbolic solutions; we may very well want to replace this handling with simple in-house symbolic linear solvers in the future if we want to extend this to supporting runtime classical parameters.
Let me mark this for merge now to shorten the dependency chain, and if we decide differently, we can revisit.
Pull Request Test Coverage Report for Build 6639515070
💛 - Coveralls |
| # Check compatibility by solving the resulting equation | ||
| sym_sol = sym.solve(equations, set(temp_symbols.values())) | ||
| for key in sym_sol: | ||
| try: | ||
| sol[str(key)] = ParameterExpression(circ_dict, sym_sol[key]) | ||
| except TypeError: | ||
| return None | ||
|
|
||
| if not sol: | ||
| # Check compatibility by solving the resulting equation. `dict=True` (surprisingly) forces | ||
| # the output to always be a list, even if there's exactly one solution. | ||
| sym_sol = sym.solve(equations, set(temp_symbols.values()), dict=True) | ||
| if not sym_sol: | ||
| # No solutions. | ||
| return None | ||
|
|
||
| for key in temp_symbols: | ||
| fake_bind[key] = sol[str(key)] | ||
| # If there's multiple solutions, arbitrarily pick the first one. | ||
| sol = { | ||
| param.name: ParameterExpression(circ_dict, to_native_symbolic(expr)) | ||
| for param, expr in sym_sol[0].items() | ||
| } | ||
| fake_bind = {key: sol[key.name] for key in temp_symbols} |
There was a problem hiding this comment.
Does it make sense to doc the potential behavioural change in an upgrade release note?
Summary
TemplateOptimizationcurrently uses Sympy internally to get access to solvers that have no equivalent in Symengine. Previously, it then did not convert the solutions to its equations back into Symengine format (if appropriate) before passing them on toParameterExpression, which could lead to different assumptions about the type of the contained objects and bugs ifParameterExpressionattempted to use Symengine-specific forms of methods on Sympy objects.Details and comments
Fix #11106.
Believe it or not, I ended up here while trying to make a sensible implementation of #7107. It turns out there was a long tail of little fixes.