Allow ParameterExpression values in TemplateOptimization pass
#6899
Conversation
… parameters, removed some excess files from another branch
…ing Parameters either in equations or symbols set in template_substitution.py
… lean on parse_expr from sympy to cast to symbols
…race to fix template use with Parameters
…tain floats for RXGates
… transpilig step followed by parameter binding
…terra into template-param-expression
…terra into template-param-expression
|
Concerning the main remaining issue here, I fixed the logic so that the binding attempts go through every (assumed) It also works with multiple parameters, and I've added a test |
Use of `_optionals.HAS_SYMENGINE` is intended to be within run-time locations; putting it at the top level of a file causes this fairly heavy library to be imported at runtime, slowing down `import qiskit` for those who won't use that functionality.
This adds several tests of the exact form produced by running the template-matching transpiler pass, including those with purely numeric circuits and those with symbolic parameters.
This fixes issues introduced recently in the PR that caused parameters to be incorrectly bound in the result. This meant that the actual numbers in the produced circuits were incorrect. This happened mostly by tracking data structures being updated at the wrong levels within loops. In addition, this commit also updates some data structures to more robust and efficient versions: - Testing whether a parameter has a clash is best done by constructing a set of parameters used in the input circuits, then testing directly on this, rather than stringifying expressions and using subsearch matches; this avoids problems if two parameters have contained names, or if more than one match is catenated into a single string. - Using a dictionary with a missing-element constructor to build the replacement parameters allows the looping logic to be simpler; the "build missing element" logic can be separated out to happen automatically.
There was a problem hiding this comment.
I think the recent changes had broken some of the core template matching - in my tests, very simple templates were inserting the wrong numeric parameters in the pass. This is really why I've been hammering on the idea of exact tests of the behaviour - I know the test suite was lacking in them before, but they really are crucial.
This PR's been going for a while now, in good part because of slowness on my part and me being away, so I've gone ahead and fixed the bugs I found in this review, added the explicit tests I'd been asking for and improved a couple of the data structures used in the matching. I'll have another look tomorrow before merging to make sure to check my own code, but unless I've made a howler in the test suite, this is hopefully good to merge.
Thanks for sticking with it - it'd be good if you could look over the new tests that I've added to make sure you agree that they all should pass (they currently do). If so, I'll be able to merge it soon.
| if _optionals.HAS_SYMENGINE: | ||
| import symengine |
There was a problem hiding this comment.
Actually symengine isn't that heavy to import as it's mostly cython and a statically linked c++ lib. It's sympy we normally have to avoid as it's super slow to import. The bigger thing is that symengine has a narrower platform support than qiskit and we can't rely on it being present, it's in the requirements list for platforms that support it but not all do.
There was a problem hiding this comment.
Oh, it actually appeared quite heavy on my laptop when I tried it - I must have been hitting a really cold cache. At any rate, these changes were unrelated to this PR logically, so I put them back in place.
| for param_exp in self.circuit_dag_dep.get_node(qc_idx).op.params: | ||
| if isinstance(param_exp, ParameterExpression): | ||
| circ_param_str += str(param_exp) |
There was a problem hiding this comment.
Using a string for this isn't ideal: it would falsely detect duplications if the template parameter was called alpha and a circuit parameter was called a, for example. It's also quite inefficient to build up a string like this, but that's probably not the limiting factor.
Instead, it's better to use a set or a dictionary (some form of hashmap), which provides constant-time lookup of individual objects. I've done that in a14dc37.
| new_param_name = "".join( | ||
| random.choice(string.ascii_lowercase) for i in range(8) | ||
| ) | ||
| sub_params[t_param] = Parameter(new_param_name) | ||
| t_param_exp = t_param_exp.assign(t_param, sub_params[t_param]) |
There was a problem hiding this comment.
This generates a new parameter every time the clash is encountered, which would likely have caused issues if the same parameter needed to be put into a circuit more than once. Better to create the new parameters only once, and retrieve them from a dictionary on subsequent accesses. This can be made quite elegant with collections.defaultdict - I've done that in a14dc37.
| sub_node_params.append(t_param_exp) | ||
| template_params.append(t_param_exp) | ||
| else: | ||
| sub_node_params.append(t_param_exp) | ||
| template_params.append(t_param_exp) |
There was a problem hiding this comment.
These appends were at the wrong level - they only got added to the lists if the parameters were expressions. That was a problem for template_params in particular, since it would have the wrong number of elements, and the subsequent zip between circuit_params and template_params would line things up incorrectly.
I added very explicit tests to show the failures in fc83156, and fixed them in a14dc37.
| if _optionals.HAS_SYMENGINE: | ||
| import symengine |
There was a problem hiding this comment.
Actually symengine isn't that heavy to import as it's mostly cython and a statically linked c++ lib. It's sympy we normally have to avoid as it's super slow to import. The bigger thing is that symengine has a narrower platform support than qiskit and we can't rely on it being present, it's in the requirements list for platforms that support it but not all do.
jakelishman
dismissed
their stale review
requested changes were made, but still needs a check over
|
Thanks @jakelishman! I can confirm that all the tests in |
jakelishman
added
Changelog: New Feature
jakelishman
changed the title
ParameterExpression values in TemplateOptimization pass
Summary
This PR adds the ability to pass circuits with unbound
Parameters into the template optimization transpilation pass. At this time, the.inverse()method used in each gate class is not fully implemented to handle the necessary inversion that may be required in certain templates.Details and comments
Since
Parameters may be used in both templates and circuits, these need to be treated separately so that the templateParameters can be replaced by those from the circuit. These are solved for intemplate_substitution.pyusingSympy, and henceParameters defined by LaTeX expressions are currently unsupported due to parsing issues.Currently, only the inverse of the
RXGateis supported due to the type-checking cases forParameterExpressions consisting ofParameters,ParameterExpressions consisting of numbers, and numbers. This issue is probably better handled in a general context instead of doing this type-checking in each gate definition.Similarly, this PR will not pass the
test_unbound_parametersunit test intest_template_matching.pybecause the inverse gate is defined as aUnitaryGatethat is currently cast to a complexnumpyarray inunitary.py.