Parameter table not updated after circuit assignment

nbronn commented 3 years ago

Information

Qiskit Terra version: 0.17.4
Python version: 3.9.5
Operating system: Darwin

What is the current behavior?

When performing a circuit assignment, _parameter_table is not updated to remove the discarded instruction, causing certain methods to fail (i.e., .copy()).

Steps to reproduce the problem

from qiskit import QuantumCircuit, QuantumRegister
from qiskit.circuit import Parameter

theta = Parameter('$\\theta$')
qr = QuantumRegister(2, 'q')
qc = QuantumCircuit(qr)
qc.rz(theta, 1)
print(qc)
print(qc._parameter_table[theta])

prints

q_0: ────────────────
     ┌──────────────┐
q_1: ┤ RZ($\theta$) ├
     └──────────────┘
[(<qiskit.circuit.library.standard_gates.rz.RZGate object at 0x7ffc7bd23eb0>, 0)]

Now, modify circuit by assignment in some way, for example by adding a control to the RZGate:

# make every gate controlled by q_0
for idx, (instr, qargs, cargs) in enumerate(qc.data):
    g = instr.control(1)
    qc.data[idx] = (g, [qr[0]] + qargs, cargs)

print(qc)
print(qc._parameter_table[theta])

prints

q_0: ───────■────────
     ┌──────┴───────┐
q_1: ┤ RZ($\theta$) ├
     └──────────────┘
[(<qiskit.circuit.library.standard_gates.rz.RZGate object at 0x7ffc7bd23eb0>, 0), (<qiskit.circuit.library.standard_gates.rz.CRZGate object at 0x7ffc7bd3e370>, 0)]

Note that there are two instructions in the parameter table, one corresponding to CRZGate that was assigned, and the original RZGate that is no longer in the circuit. This causes errors when trying to map between parameter tables, for example the .copy() method:

print('Original RZ instruction id is '+str(id(qc._parameter_table[theta][0][0])))
qc.copy()

prints

Original RZ instruction id is 140722385862320
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
<ipython-input-4-79e10aadd73c> in <module>
      1 print('Original RZ instruction id is '+str(id(qc._parameter_table[theta][0][0])))
----> 2 qc.copy()

~/anaconda3/envs/qiskit27/lib/python3.9/site-packages/qiskit/circuit/quantumcircuit.py in copy(self, name)
   1749                         for id_, instr in instr_instances.items()}
   1750 
-> 1751         cpy._parameter_table = ParameterTable({
   1752             param: [(instr_copies[id(instr)], param_index)
   1753                     for instr, param_index in self._parameter_table[param]]

~/anaconda3/envs/qiskit27/lib/python3.9/site-packages/qiskit/circuit/quantumcircuit.py in <dictcomp>(.0)
   1750 
   1751         cpy._parameter_table = ParameterTable({
-> 1752             param: [(instr_copies[id(instr)], param_index)
   1753                     for instr, param_index in self._parameter_table[param]]
   1754             for param in self._parameter_table

~/anaconda3/envs/qiskit27/lib/python3.9/site-packages/qiskit/circuit/quantumcircuit.py in <listcomp>(.0)
   1750 
   1751         cpy._parameter_table = ParameterTable({
-> 1752             param: [(instr_copies[id(instr)], param_index)
   1753                     for instr, param_index in self._parameter_table[param]]
   1754             for param in self._parameter_table

KeyError: 140722385862320

In this case, we see that the error was caused by instruction 140722385862320, which corresponds to the original RZGate instruction.

What is the expected behavior?

The parameter table should be updated upon assignment.

Suggested solutions

Remove overwritten instructions from the parameter table.

Cryoris commented 3 years ago

Thanks for the report! I believe we had similar issues with append/compose but fixed them there as they are the primary methods for modifying the circuit.

@kdk Since the data attribute is used a lot internally (e.g. to append) do you have an idea of whether adding a parameter table update here could have a performance impact? If no it might be good that we add a safeguard, otherwise we should maybe just clearly state that it doesn't handle the parameters properly 🤔

kdk commented 3 years ago

@kdk Since the data attribute is used a lot internally (e.g. to append) do you have an idea of whether adding a parameter table update here could have a performance impact? If no it might be good that we add a safeguard, otherwise we should maybe just clearly state that it doesn't handle the parameters properly 🤔

The fix here shouldn't have a substantial performance impact. All of our existing tooling (append,compose) should be updating the circuit through QuantumCircuit._data and manually handling ParameterTable updates. The bug here is with the validation we automatically do for setting/adding instructions through QuantumCircuit.data (without the _).

In https://github.com/Qiskit/qiskit-terra/blob/2eee566/qiskit/circuit/quantumcircuitdata.py#L59 , we already update the ParameterTable to account for the incoming instructions, but neglect to remove ParameterTable entries for the replaced instructions, which I think is the source of this bug.

(It would be good if at the same time we can update .insert to likewise update the ParameterTable.)

TakahitoMotoki commented 2 years ago

Can I work on this problem ? It is the first time for me to contribute to Qiskit, but I would be happy if I could try.

javabster commented 2 years ago

Sure thing, assigned to you @TakahitoMotoki! Ler us know if you need any help 😄

TakahitoMotoki commented 2 years ago

Thank you very much ,@javabster !

TakahitoMotoki commented 2 years ago

I have one question about a code test.

According to CONTRIBUTING.md, I need to run "tox -epy310" and "tox -elint" to check soundness and style of the modified code.

However, it seems that I cannot run tox command since my laptop is Macbook Pro with ARM processor. Though I am trying to figure out the solution, is there any alternative way for testing?

javabster commented 2 years ago

Hi @TakahitoMotoki I'm not aware that ARM processors cause issues with tox. As we currently don't yet support 3.10 I'd recommend trying tox -epy39 instead. Also for linting tox -eblack should work quicker than tox -elint and automatically fixes most linting errors. If you're still facing issues could you share the error message you're getting so we can help more.

Also might be worth making sure you have tox installed 😄

TakahitoMotoki commented 2 years ago

@javabster Thank you for your kind support! I switched python from 3.10 to 3.9, then "tox -eblack" successfully worked on my PC.

However, I still got an error when executing "tox -epy39". I tried to build jaxlib from source, but it didn't work. (https://jax.readthedocs.io/en/latest/developer.html)

Followings are error messages. Thank you again for your kind support.

python version: 3.9.7 Processor: Apple M1

RuntimeError: This version of jaxlib was built using AVX instructions, which your CPU and/or operating system do not support. You may be able work around this issue by building jaxlib from source.

================================================================================ The above traceback was encountered during test discovery which imports all the found test modules in the specified test_path. ERROR: InvocationError for command /Users/takahitomotoki/Qiskit/Dev/qiskit-terra/.tox/py39/bin/stestr run (exited with code 100) __ summary __ ERROR: py39: commands failed

javabster commented 2 years ago

ok so this looks like an issue with jax not tox. It seems others have had the same issue, you could try some of the solutions people have mentioned here: https://github.com/google/jax/issues/5501

TakahitoMotoki commented 2 years ago

@javabster Thank you!

TakahitoMotoki commented 2 years ago

@javabster I could solve the jax problem above (I could pass the tox test!) and finally created a pull request. I am really grateful for your help.

Goldslate commented 2 years ago

Could I please try and fix the code? I am a newbie but I would like to try and learn.

Goldslate commented 2 years ago

I don't know if this will reach anyone on here but I think it was an issue of control and target. Here's the code I wrote and please help me if it's wrong (which, I'm positive, it is)

Code: from qiskit import * from qiskit import QuantumCircuit, QuantumRegister from qiskit.circuit import Parameter

theta = Parameter('$\theta$') qr = QuantumRegister(2, 'q') qc = QuantumCircuit(qr) qc.rz(theta, 1) qc.rz(0, 1) print(qc) print(qc._parameter_table[theta])

for idx, (instr, qargs, cargs) in enumerate(qc.data): g = instr.control(1) qc.data[idx] = (g, [qr[0]] + qargs, cargs)

print(qc) print(qc._parameter_table[theta])

%matplotlib inline qc.draw(output = 'mpl')

P.S. The only thing I added here was the 8th line and the mpl plot.

Qiskit / qiskit