Qiskit / qiskit

Qiskit is an open-source SDK for working with quantum computers at the level of extended quantum circuits, operators, and primitives.
https://www.ibm.com/quantum/qiskit
Apache License 2.0
5.2k stars 2.36k forks source link

`FakePrague` list of supported operations is incorrect #9983

Closed jlapeyre closed 1 year ago

jlapeyre commented 1 year ago

Looking at this list:

from qiskit.providers.fake_provider import FakePrague
backend = FakePrague()
list(zip(backend.operation_names, [type(x) for x in backend.operations]))
[('id', qiskit.circuit.library.standard_gates.i.IGate),
 ('rz', qiskit.circuit.library.standard_gates.rz.RZGate),
 ('sx', qiskit.circuit.library.standard_gates.sx.SXGate),
 ('x', qiskit.circuit.library.standard_gates.x.XGate),
 ('cz', qiskit.circuit.gate.Gate),
 ('reset', qiskit.circuit.reset.Reset),
 ('measure', qiskit.circuit.measure.Measure),
 ('delay', qiskit.circuit.delay.Delay)]

you see that the entry for cz is not an instance of CZGate. This is apparently the origin of the bug referenced in #9935 #9937 #9982 .

The gate with name cz in the list above is not recognized as a CZ gate when doing unitary synthesis here https://github.com/Qiskit/qiskit-terra/blob/971a203deb33bdc309b1467d00e39efc5eda8936/qiskit/transpiler/passes/synthesis/unitary_synthesis.py#L646 and here https://github.com/Qiskit/qiskit-terra/blob/971a203deb33bdc309b1467d00e39efc5eda8936/qiskit/transpiler/passes/synthesis/unitary_synthesis.py#L677

Furthermore:

In [37]: badcz = backend.operations[4]

In [38]: badcz.definition  # None

In [39]: badcz.to_matrix()  # Raises error CircuitError: "to_matrix not defined for this <class 'qiskit.circuit.gate.Gate'
mtreinish commented 1 year ago

That's a really good catch and yeah that's definitely the underlying cause of the issue. It still points to a bug in the synthesis path because we should be able to run transpile() without erroring in the case of a custom 2q basis gate. But that's independent of fixing the target construction for the fake backends.

The root cause is the custom target builder for the fake backends: https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/providers/fake_provider/utils/backend_converter.py#L30 hasn't been updated to handle the new basis gates. But instead of hardcoding that we should probably use the more general https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/providers/backend_compat.py#L33 version used in the backend converter. If we just change the usage to the more general function here: https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/providers/fake_provider/fake_backend.py#L171 it should solve this and also be more future proof against any other new gates being used.

That being said the 2 things off the top of my head which are different between the two functions are that the fake backend version works with the raw deserialized json dictionary. We'd have to create BackendConfiguration,BackendProperties, and PulseDefault files from the payloads (this is why the two functions weren't combined at the time of the compat converter's introduction). The second is I believe the fake provider copy has special handling for lazy loading pulse schedules from the defaults payload we would need to ensure this persists. The lazy loading is important because generating the pulse schedules is very heavy and adds a lot of overhead to just using a backend which shouldn't ever be needed unless you're working with pulses.

jlapeyre commented 1 year ago

without erroring in the case of a custom 2q basis gate

Right a gate defined by a user or other part of Qiskit should work if it satisfies some requirements. But if it's intended to be a controlled gate, there should be a way to determine this. I don't know if is_controlled is a proxy for (or equivalent to) "entangling gate" . If we are doing unitary synthesis it seems reasonable to require at least one entangling gate be available. Looking at the literature, it's clear that the condition checked for in is_controlled is that the gate is locally equivalent to controlled U for some 1q U. So this is really checking for controlled gates and not entangling gates in general. A basis set doesn't need a controlled gate, but should have an entangling gate.

Suppose a basis set has a non-control entangling gate. Does it still make sense for it to be sent to DefaultUnitarySynthesis? If so, then I guess the correct thing to do is simply skip those synthesizers that require a controlled gate. In this path at least, this appears to be all of them.

levbishop commented 1 year ago

The logic of the file is hard for me to follow but surely we should still try to synthesize even without a controlled basis - the supercontrolled decomposer works for eg iSWAP so i think just skip adding XXDecomposer to the list

jlapeyre commented 1 year ago

Currently the logic in unitary_synthesis.py creates a list of decomposers to try. If there are any super-controlled gates in the basis, it will unconditionally include decomposers that use these.

To clarify, the cases that are raising these issues are really anomalous. There are no entangling gates at all in the basis set. So you can't synthesize gates for circuits that have entangling gates, only, possibly, circuits with no entangling gates. It's not clear whether or why we should support this. But in fact there is a test in the test suite that relies on synthesizing a 2q identity gate with no entangling gates in the basis.

But I realize now that #9994 is not good enough. It will throw an error if you try to synthesize a non-trivial circuit with no controlled gates. But there may be super-controlled gates in the original basis and decomposers for them in the list. All decomposers are tried and the best result is chosen. The decomposer that take the empty list of controlled gates will throw an error even if the decomposer using super-controlled gates succeeds. To be clear this was the situation before #9994 as well. But there was no test to that triggered it.... there must be some test with a basis set that includes iSWAP, but not controlled gates. I'll check...

jlapeyre commented 1 year ago

here must be some test with a basis set that includes iSWAP, but not controlled gates.

Evidently not. I made a target with iSwap and no controlled gates and tried to synthesize CX gate converted to a unitary matrix. It raised the index error in XXDecomposer. I'll talk about options to fix it in an issue.