Fix handling of multiple available target ops in UnitarySynthesis #7795
Conversation
We recently added support for working natively with a Target to the UnitarySynthesis pass in Qiskit#7775. However, in that PR we took a naive approach to dealing with the KAK gate for decomposition which would result in the pass not correctly generating an output circuit. Previously, it would just pick the first KAK gate that was present in the target whether or not it was the best choice available. There was also a bug in Qiskit#7775 where it would only look at the cx error rate instead of the error rate for the selected kak gate. This bug could result in cases of the output of the unitary synthesis would be considered invalid by later passes because it didn't properly respect the constraints of the backend. This commit fixes these issues by instead considering all combinations of 1q and kak gate in the target and picking the combination with the lowest error rate available on the selected qubits. Then the unit testing for the pass is expanded to cover the use of the target to ensure we're testing the full path with a target specified.
mtreinish
added
priority: high
Changelog: None
Pull Request Test Coverage Report for Build 2067085606
💛 - Coveralls |
The test assertions in the new tests were previously flawed and would evaluate as True if there were no gates for the in the circuit we were checking. This commit fixes the tests so we're actually asserting things correctly.
This commit reorganizes the internal helper functions to split the target and non-target paths to be completely separate. Then the target path adds a cache for the 2q decomposer to use for the specified qubits so we don't have to do the lookup more than once on subsequent runs.
| def __init__(self): | ||
| super().__init__() | ||
| self._decomposer_cache = {} | ||
|
|
There was a problem hiding this comment.
For another PR: I guess we should document what the lifetime of an instance of a plugin class is for the interface - the implementation is that it lives for the lifetime of a pass of the outer UnitarySynthesis, which is one run over the DAG, but we should be clear about it so others can cache like this if they want to. I also wonder if we should consider re-organising the interface such that the free-form configuration dictionary gets passed to the __init__, not the run (or both).
There was a problem hiding this comment.
Well it could be more than one run over a dag, it depends specifically how the pass manager is constructed and used. For example, with transpile() it could be used multiple times on each iteration of the optimization loop (either in opt level 3 or if the translation method is set to synthesis). This means the plugin instance for that synthesis pass will be called multiple times depending on how many iterations the optimization loop needs. (which is why I added the cache because it could make a noticeable difference in those cases).
That being said I agree, we might need to revisit how we manage the flow of data between the pass and plugins. When we originally designed it plugins were intended to be more of a black box stateless function, you give it a unitary and it gives you back a dagcircuit. But in practice so far I think all the plugins (now with this pr, previously default was stateless) have some level of state and/or caching they do out of band. We should open an issue on it to figure out how to evolve the interface to support this.
There was a problem hiding this comment.
Opened #7843 to track this.
| "rxx": RXXGate(pi / 2), | ||
| "ecr": ECRGate(), | ||
| "rzx": RZXGate(pi / 4), # typically pi/6 is also available |
There was a problem hiding this comment.
can we hope to fix these magic angle values with Target? It would require to not rely on string names but rather whatever RZX(theta) instruction that the backend has available.
There was a problem hiding this comment.
I opened an issue about this in #7797 we need to do an overhaul of how we look for gates in the transpiler. I have PR open on start this with: #7807 which gives us a boolean check if a operation class and parameter value are supported by a target, but we also need a lookup method to get the name from a class and parameter value too. Once those are in place then we'll need to update all the places which are doing things like this to instead check the target and use that instead (which will require a good chunk of work I suspect).
| backend = FakeMumbaiV2() | ||
| synth_pass = UnitarySynthesis(target=backend.target) | ||
| tqc = synth_pass(circ) | ||
| tqc_index = {qubit: index for index, qubit in enumerate(tqc.qubits)} | ||
| self.assertGreaterEqual(len(tqc.get_instructions("rzx")), 1) |
There was a problem hiding this comment.
I think FakeMumbaiV2 has both RZX(pi/4) and RZX(pi/6)... do both of them get used here or because we have hardcoded pi/4 in the KAK_GATE_NAMES, only pi/4 gets used?
There was a problem hiding this comment.
I think they actually both get used but I think it's actually a bug in the XXDecomposer though, because we never tell it anything about which specific angles are available it just assumes that pi/6 is available.
Co-authored-by: Jake Lishman <[email protected]>
Summary
We recently added support for working natively with a Target to the
UnitarySynthesis pass in #7775. However, in that PR we took a naive
approach to dealing with the KAK gate for decomposition which would
result in the pass not correctly generating an output circuit.
Previously, it would just pick the first KAK gate that was present in
the target whether or not it was the best choice available. There was
also a bug in #7775 where it would only look at the cx error rate
instead of the error rate for the selected kak gate. This bug could
result in cases of the output of the unitary synthesis would be
considered invalid by later passes because it didn't properly respect
the constraints of the backend. This commit fixes these issues by
instead considering all combinations of 1q and kak gate in the target
and picking the combination with the lowest error rate available on the
selected qubits. Then the unit testing for the pass is expanded to cover
the use of the target to ensure we're testing the full path with a
target specified.
Details and comments