Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 6633: Update _parameter_table after circuit assignment #7434

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
ace8f24
Update _parameter_table after circuit assignment
TakahitoMotoki Dec 5, 2021
471f1aa
Update _parameter_table after circuit assignment
TakahitoMotoki Dec 14, 2021
da1a64f
Update _parameter_table after circuit assignment
TakahitoMotoki Dec 20, 2021
68b157b
Update _parameter_table after circuit assignment
TakahitoMotoki Dec 22, 2021
a540ec1
Merge branch 'main' into issue_6633
TakahitoMotoki Dec 22, 2021
4551c3d
Merge branch 'main' into issue_6633
TakahitoMotoki Dec 22, 2021
b50f8a8
Update _parameter_table after circuit assignment
TakahitoMotoki Dec 30, 2021
f5be2ad
Merge branch 'main' into issue_6633
TakahitoMotoki Dec 30, 2021
5c9ff04
Update _parameter_table after circuit assignment
TakahitoMotoki Dec 30, 2021
05a13a5
Merge branch 'issue_6633' of https://github.com/TakahitoMotoki/qiskit…
TakahitoMotoki Dec 30, 2021
ed4395e
Merge branch 'main' into issue_6633
TakahitoMotoki Jan 5, 2022
d318a18
Update _parameter_table after circuit assignment
TakahitoMotoki Jan 5, 2022
1db7729
Update _parameter_table after circuit assignment
TakahitoMotoki Jan 5, 2022
b2837b1
Merge branch 'main' into issue_6633
TakahitoMotoki Jan 14, 2022
e16f51c
Add a test to test_circuit_data.py
TakahitoMotoki Jan 14, 2022
e8e54a9
Add a release note
TakahitoMotoki Jan 14, 2022
b5969ad
Merge branch 'main' into issue_6633
TakahitoMotoki Feb 2, 2022
3c352cc
Add public function to ParameterTable.py
TakahitoMotoki Feb 3, 2022
86dc448
Update tox -eblack
TakahitoMotoki Feb 6, 2022
9592520
Modify test case
TakahitoMotoki Feb 10, 2022
2832122
run tox -eblack
TakahitoMotoki Feb 10, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions qiskit/circuit/quantumcircuitdata.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

from qiskit.circuit.exceptions import CircuitError
from qiskit.circuit.instruction import Instruction
from .parameter import Parameter


class QuantumCircuitData(MutableSequence):
Expand Down Expand Up @@ -54,10 +55,46 @@ def __setitem__(self, key, value):
self._circuit._check_qargs(qargs)
self._circuit._check_cargs(cargs)

circ_data = {}
param = {"old": 0, "new": 0}
circ_data["old"] = self._circuit._data
self._circuit._data[key] = (instruction, qargs, cargs)
circ_data["new"] = self._circuit._data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to use dictionaries for this. It's much clearer to use two separate variables. This also does not do what you think - you're saving the same reference to the list into both old and new, and so there are no circumstances in which circ_data["old"] will ever be different from circ_data["new"], because they're the exact same object.


self._circuit._update_parameter_table(instruction)

if len(circ_data["old"][key][0].params) > 0:
if isinstance(circ_data["old"][key][0].params[0], Parameter):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't handle ParameterExpression, which is the base class - there may be more than one parameter in any given position.

param["old"] = circ_data["old"][key][0].params[0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only checks the first parameter of an instruction, not all of the parameters.


if len(circ_data["new"][key][0].params) > 0:
if isinstance(circ_data["new"][key][0].params[0], Parameter):
param["new"] = circ_data["new"][key][0].params[0]

param_count = -1
if isinstance(param["old"], Parameter):
for idx, data in enumerate(circ_data["old"]):
if idx <= key:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be better done by only enumerating the items you are interested in, rather than looping through everything and skipping the last set.

if len(data[0].params) > 0:
if data[0].params[0] == param["old"]:
param_count = param_count + 1
else:
pass
self._circuit._parameter_table[param["old"]].pop(param_count)

param_count = -1
if isinstance(param["new"], Parameter):
for idx, data in enumerate(circ_data["new"]):
if idx <= key:
if len(data[0].params) > 0:
if data[0].params[0] == param["new"]:
param_count = param_count + 1
else:
pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole section is difficult to read, to the point that I'm not confident that I can review it for correctness. I suggested a few simplifications above, but really I think a lot of this should be in quite a different form within ParameterTable, not here. I think that a lot of this information could be read much faster out of ParameterTable, rather than iterating through all the circuit data twice, but I'm not certain.

added_element = self._circuit._parameter_table[param["new"]][-1]
self._circuit._parameter_table[param["new"]].insert(param_count, added_element)
self._circuit._parameter_table[param["new"]].pop()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why update the parameter table earlier if we need to remove what it did and re-arrange it?

More importantly, though, all of this modification of the internal data structures of ParameterTable shouldn't happen here in an unrelated class - I think a more complete mechanism for this needs to have ParameterTable gain some public methods that can be used to remove parameters from the table. However, that comes with the reference counting issues that I mentioned previously; this PR also doesn't account for the parameter table needing to remove a Parameter key if the last reference to a Parameter is removed from the circuit. If the same instruction (referentially) is put in the circuit more than once using this method, the number of elements in ParameterTable will likely be incorrect, and removals might not make sense.


def insert(self, index, value):
self._circuit._data.insert(index, None)
self[index] = value
Expand Down
1 change: 1 addition & 0 deletions qiskit/test/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ class BaseTestCase(testtools.TestCase):
assertRaises = unittest.TestCase.assertRaises
assertEqual = unittest.TestCase.assertEqual


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version of black you have is out-of-date - we updated the tox configuration a couple of months ago, including a bump of black. If you've merged in the latest changes from the main branch, then you should be able to run tox -r -e black to regenerate the tox environment, which should install the correct version of black, and fix the discrepancy between your local copy and our CI.

else:

class BaseTestCase(unittest.TestCase):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
fixes:
- |
Fixed a bug when ``QuantumCircuit._parameter_table`` is inconsistent with
``QuantumCircuit.data`` after a circuit assignment.
In __setitem__ of QuantumCiruitData, for each new entry, the old element
is removed and the new element is added properly.
16 changes: 16 additions & 0 deletions test/python/circuit/test_circuit_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -425,3 +425,19 @@ def test_param_gate_instance(self):
qc0_instance = qc0._parameter_table[b][0][0]
qc1_instance = qc1._parameter_table[a][0][0]
self.assertNotEqual(qc0_instance, qc1_instance)

def test_parameter_table_is_updated(self):
"""Verify that circuit._parameter_table is consistent with circuit.data."""
qr = QuantumRegister(1, "q")
qc = QuantumCircuit(qr)

a = Parameter("a")
qc.h(0)
qc.ry(a, 0)
qc.rz(a, 0)

qc.data[0] = (RXGate(a), [qr[0]], [])
qc.data[1] = (RXGate(a), [qr[0]], [])
qc.data[2] = (HGate(), [qr[0]], [])

qc.copy()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test has no assertions - it is only testing that nothing crashes. Tests also need to have positive assertions, that the output is correct. In every case, we should also test that:

  • the circuit is equal to the form we expect it to be
  • parameter assignment works, and binds all parameters

There are also quite a few special cases, that we should be sure to test as well:

  • an instruction is added that adds a parameter to the circuit that wasn't previously there
  • an instruction is changed such that the total number of instructions in the circuit that have parameters is different
  • instructions with more than one parameter (e.g. u) work
  • instructions that take a ParameterExpression work
  • if the last reference to a Parameter is removed from a circuit with this method, it is no longer in the parameter table
  • the table is updated correctly if the same instruction (referentially) is passed in more than once

That list might not be complete, so other ideas are good too.