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.05k stars 2.33k forks source link

Loading Qpy file with initialized quantum circuit throws "QiskitError: 'Sum of amplitudes-squared is not 1, but 0.0.'" #11158

Closed ljkitt closed 10 months ago

ljkitt commented 10 months ago

Environment

What is happening?

When trying to load a Qpy file that was made from a circuit with an initialize instruction, it produces the following error:

Traceback (most recent call last):
  Cell In[22], line 13
    qc = qpy.load(fd)[0]
  File /opt/conda/lib/python3.10/site-packages/qiskit/qpy/interface.py:269 in load
    loader(
  File /opt/conda/lib/python3.10/site-packages/qiskit/qpy/binary_io/circuits.py:1092 in read_circuit
    _read_instruction(file_obj, circ, out_registers, custom_operations, version, vectors)
  File /opt/conda/lib/python3.10/site-packages/qiskit/qpy/binary_io/circuits.py:301 in _read_instruction
    gate = gate_class(params)
  File /opt/conda/lib/python3.10/site-packages/qiskit/extensions/quantum_initializer/initializer.py:58 in __init__
    self._stateprep = StatePreparation(params, num_qubits, normalize=normalize)
  File /opt/conda/lib/python3.10/site-packages/qiskit/circuit/library/data_preparation/state_preparation.py:107 in __init__
    raise QiskitError(f"Sum of amplitudes-squared is not 1, but {norm}.")
QiskitError: 'Sum of amplitudes-squared is not 1, but 0.0.'

How can we reproduce the issue?

Run the following:

import qiskit
from qiskit import QuantumCircuit, ClassicalRegister, QuantumRegister
from qiskit import qpy

qr = QuantumRegister(8, name='qr')
cr = ClassicalRegister(8, name='cr')
qc = QuantumCircuit(qr, cr, name='qc')
qc.initialize(0, qc.qubits)

with open('tmp.qpy', 'wb') as fd:
    qpy.dump(qc, fd)
with open('tmp.qpy', 'rb') as fd:
    qc = qpy.load(fd)[0]

What should happen?

Qpy should be able to load the circuit from the file generated.

Any suggestions?

No response

SoranaAurelia commented 10 months ago

The problem seems to be stemming from how the initialize (more precisely, the Statepreparation) instruction is saved and then retreived. When initialized with an int as an argument, in the end the gate produces a list with one element with the desired value:

https://github.com/Qiskit/qiskit/blob/02cb814e7669b8035924399b935b003032e1f7b3/qiskit/circuit/library/data_preparation/state_preparation.py#L111

And this is how the instruction is saved (containing a list with only one element - the parameter sent). When this is retreived and the StatePreparation instruction called, it assumes that this is a list that should contain the "vector of complex amplitudes to initialize to". However, since the parameters saved is a list with one element (and that being 0) it gives the error that this is not a valid list.

When running the provided example, when the circuit is loaded, _from_label and _from_int are false (since it reads a list with one element). https://github.com/Qiskit/qiskit/blob/02cb814e7669b8035924399b935b003032e1f7b3/qiskit/circuit/library/data_preparation/state_preparation.py#L99-L108

This is also the reason that for initialize instruction, when using the constructor with the list, the bug does not occur.

# qc- quantum circuit with 2 qubits
qc.initialize([0, 1, 0, 0], qr)
with open('tmp.qpy', 'wb') as fd:
    qpy.dump(qc, fd)
with open('tmp.qpy', 'rb') as fd:
    qc = qpy.load(fd)[0]

And when using initialize with 1, the error is given because of the "length of the list", rather than the amplitude verification. The code:

# qc=....
qc.initialize(1, qr)
with open('tmp.qpy', 'wb') as fd:
    qpy.dump(qc, fd)
with open('tmp.qpy', 'rb') as fd:
    qc = qpy.load(fd)[0]

gives the error: qiskit.exceptions.QiskitError: 'Desired statevector length not a positive power of 2. since it passed the if from line 103 and not the following check (in _get_num_qubits) since it finds a list of length 1 https://github.com/Qiskit/qiskit/blob/02cb814e7669b8035924399b935b003032e1f7b3/qiskit/circuit/library/data_preparation/state_preparation.py#L110

So a quick workaround is using initialize with a list or label rather then int. I'll investigate further to see what good solutions are available

SoranaAurelia commented 10 months ago

* note on previous comment: using labels also produces the bug, since appending an initialize instruction with a label (for example '010') in the end produces an instruction with a list as params ([CircuitInstruction(operation=Instruction(name='initialize', num_qubits=3, num_clbits=0, params=['0', '1', '0']), qubits=...)]) so when it gets put again in the circuit, the same error occurs, being interpreted as a list with amplitudes, rather then the labels.

Since the way the Instruction is build (the params is always a list), I suppose that one easy solution is to take this difference in parameter initialization when the circuit is loaded back from the file. That is, in the _read_instruction function when treating this case: https://github.com/Qiskit/qiskit/blob/2818cf05b6a4680b7ff6241fe2375554f84df594/qiskit/qpy/binary_io/circuits.py#L322-L330

a quick fix would be treating which case of instatiation is based on params here:

if gate_name in {"Initialize", "StatePreparation"}:
    if isinstance(params[0], str): # or all(isinstance(param, str) for param in params)
        # the StatePreparation is done by the given labels
        gate = gate_class("".join(label for label in params))
    elif instruction.num_parameters == 1:
        # the StatePreparation is done through an integer indicating which qubits to initialize
        gate = gate_class(int(params[0].real), instruction.num_qargs)
    else:
        # the StatePreparation is done through a list of complex amplitudes
        gate = gate_class(params)

One could also modify when the instruction is saved in the qpy file or how params are stored in the StatePreparation instruction, as to make the writing/reading more independent of initialization. However, I recognize I am not very familiar with the code just yet, so a more experienced opinion would be appreciated.

jakelishman commented 10 months ago

@SoranaAurelia: what you're saying in the second comment is sensible. StatePreparation is really quite weird in how it handles parameters, and I think the only way it'll work in its current structure is to have QPY infer that a length-one parameter list should be reduced to an integer on reconstruction. It's unpleasant and non-standard, but short of rewriting a lot of StatePreparation code (which would have a knock-on effect of breaking Aer's simulation of the instruction), it's the best we've got.

Would you like to work on the issue?

SoranaAurelia commented 10 months ago

Yes I would :) So just to be sure, going with the "quick fix" is the way to go here, right?

jakelishman commented 10 months ago

Yeah, exactly.