Issue 6633: Update _parameter_table after circuit assignment #7434
Conversation
There was a problem hiding this comment.
Hello, thanks for looking at this!
At the moment, I'm quite worried that this implementation will have really big performance implications - on every modification to the circuit data, it iterates through the entire circuit's data twice over. The current data structures in ParameterTable are really only designed for additive usage; they can't really handle modification, because there's no way to tell how many different data elements use each entry in the table. I think we would need to introduce some form of reference counting to the elements of ParameterTable to handle this somewhat efficiently.
|
Also appears to be blocked on #7457 |
|
@jakelishman
|
|
In addition to @jakelishman's comments, a few more things from me:
|
|
@javabster
|
There was a problem hiding this comment.
I don't think that this new form has addressed the efficiency concerns from before; it still loops over the entire circuit data twice, even though this implementation of __setitem__ only allows replacements. Since the number and position of all the instructions cannot change, we shouldn't need to loop over everything each time to determine things.
I think a lot of the parameter handling here should be done in ParameterTable, not in this structure. We shouldn't be mutating private attributes of objects that aren't ourselves - it's very fragile to code changes, and it's difficult to maintain, because it's never clear which object is responsible for the mutations. I think some of this will be made easier by #7408; the ordering of references in the table will become less important, so searching it will be much faster.
This whole QuantumCircuitData object is a slight exception, because it mutates private methods of QuantumCircuit, but in an ideal world, this object would be defined inside QuantumCircuit itself, because it's really not separate at all.
qiskit/circuit/quantumcircuitdata.py
Outdated
| 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 |
There was a problem hiding this comment.
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.
qiskit/circuit/quantumcircuitdata.py
Outdated
| if len(circ_data["old"][key][0].params) > 0: | ||
| if isinstance(circ_data["old"][key][0].params[0], Parameter): | ||
| param["old"] = circ_data["old"][key][0].params[0] |
There was a problem hiding this comment.
This only checks the first parameter of an instruction, not all of the parameters.
qiskit/circuit/quantumcircuitdata.py
Outdated
| for idx, data in enumerate(circ_data["old"]): | ||
| if idx <= key: |
There was a problem hiding this comment.
This would be better done by only enumerating the items you are interested in, rather than looping through everything and skipping the last set.
qiskit/circuit/quantumcircuitdata.py
Outdated
| if len(circ_data["old"][key][0].params) > 0: | ||
| if isinstance(circ_data["old"][key][0].params[0], Parameter): | ||
| param["old"] = circ_data["old"][key][0].params[0] | ||
|
|
||
| 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: | ||
| 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 |
There was a problem hiding this comment.
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.
qiskit/circuit/quantumcircuitdata.py
Outdated
|
|
||
| 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): |
There was a problem hiding this comment.
This doesn't handle ParameterExpression, which is the base class - there may be more than one parameter in any given position.
qiskit/circuit/quantumcircuitdata.py
Outdated
| 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() |
There was a problem hiding this comment.
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.
qiskit/test/base.py
Outdated
| @@ -62,6 +62,7 @@ class BaseTestCase(testtools.TestCase): | |||
| assertRaises = unittest.TestCase.assertRaises | |||
| assertEqual = unittest.TestCase.assertEqual | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
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.
| 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() |
There was a problem hiding this comment.
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
ParameterExpressionwork - if the last reference to a
Parameteris 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.
|
@jakelishman Thus, I want to set direction with this issue. I think the solution to this problem looks like…
I am really grateful for your help. |
Pull Request Test Coverage Report for Build 1697828587
💛 - Coveralls |
TakahitoMotoki
requested review from
ikkoham,
jyu00,
manoelmarques,
nkanazawa1989,
nonhermitian and
woodsp-ibm
as code owners
|
I made quite a big modification. I am really sorry for troubling you. Problem (Copy from PullReq statement)When data property in QuantumCircuit Reason (Copy from PullReq statement)An element of SolutionMy idea is
About test codeI added a new test in test/python/circuit/test_circuit_data.py.
|
HuangJunye
added
the
Community PR
|
@TakahitoMotoki Sorry for slow response. Are you still working on this PR? There are a few conflicts with the main branch. Can you please resolve them? Please let us know if you need help. |
|
Closing this PR as contributor is no longer responsive. |
Summary
The problem is data property in QuantumCircuit object is inconsistent with ParameterTable when data property is updated by
__setitem__in quantumcircuitdata.py. In this modification, ParameterTable is cleared and reconstruct from the updated data property.Details and comments
Problem
When data property in QuantumCircuit
qc.datais set via__setitem__in quantumcircuitdata.py,qc.databecomes inconsistent withqc._parameter_table.Reason
qc.dataelement is updated in__setitem__but a new element is added to qc._parameter_table by_update_parameter_table()instead of updating an existing element.Solution
My idea is
qc.data.qc.dataand instruction.qc._parameter_table.from the newqc.data`.Fixes #6633