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.

Sign up for GitHub

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

Jump to bottom

Fix CUGate parameter assignment issues #9118

Closed
wants to merge 2 commits into from
Closed

Fix CUGate parameter assignment issues #9118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9727f91
Changed the qiskit.CUGate.params, both property and setter, to handle…
rrodenbusch Nov 10, 2022
6eced43
fix merge
rrodenbusch May 10, 2023
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
1 change: 1 addition & 0 deletions qiskit/circuit/library/standard_gates/equivalence_library.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1161,6 +1161,7 @@
lam = Parameter("lam")
cu3_to_cu = QuantumCircuit(q)
cu3_to_cu.cu(theta, phi, lam, 0, 0, 1)
_sel.add_equivalence(CU3Gate(theta, phi, lam), cu3_to_cu)

# XGate
#
Expand Down
40 changes: 16 additions & 24 deletions qiskit/circuit/library/standard_gates/u.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import numpy
from qiskit.circuit.controlledgate import ControlledGate
from qiskit.circuit.gate import Gate
from qiskit.circuit.parameterexpression import ParameterValueType
from qiskit.circuit.parameterexpression import ParameterValueType, ParameterExpression
from qiskit.circuit.quantumregister import QuantumRegister
from qiskit.circuit.exceptions import CircuitError

Expand Down Expand Up @@ -273,33 +273,25 @@ def __array__(self, dtype=None):

@property
def params(self):
"""Get parameters from base_gate.

Returns:
list: List of gate parameters.

Raises:
CircuitError: Controlled gate does not define a base gate
"""
if self.base_gate:
# CU has one additional parameter to the U base gate
return self.base_gate.params + self._params
else:
raise CircuitError("Controlled gate does not define base gate for extracting params")
"""return CU params."""
return self._params

@params.setter
def params(self, parameters):
"""Set base gate parameters.

Args:
parameters (list): The list of parameters to set.

Raises:
CircuitError: If controlled gate does not define a base gate.
"""
# CU has one additional parameter to the U base gate
self._params = [parameters[-1]]
"""Set CUGate params [theta,phi,lam,gamma] and base_gate(UGate) params [theta,phi,lam]"""
self._params = []
for single_param in parameters:
if isinstance(single_param, ParameterExpression):
self._params.append(single_param)
else:
self._params.append(self.validate_parameter(single_param))
Comment on lines +284 to +287
Copy link
Member

Choose a reason for hiding this comment

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

I think this if/else might be unnecessary? Gate.validate_parameter has handling for ParameterExpression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The if / else logic came from the setter Instruction.params; the method that was being inherited. I saw logic with and without the if / else and chose the one that seemed the lowest risk. Is it useful to keep here for the instantiation process of a Gate? I know validation happens during the bind_parameters and assign_parameters process but I did not see any validation during instantiation.

I like the idea of adding the U4Gate since it would seem to complete the set.

if self.base_gate:
self.base_gate.params = parameters[:-1]
else:
raise CircuitError("Controlled gate does not define base gate for extracting params")

def __deepcopy__(self, _memo=None):
"""Include the parameters in the copy"""
cpy = super().__deepcopy__(_memo=_memo)
cpy.params = self.params.copy()
return cpy
Comment on lines +292 to +297
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing the reason this is necessary is because the parameter assignment isn't actually writing out to the base_gate, because QuantumCircuit._assign_parameter modifies the Gate.params object by setting an item in it. That dodges the params setter, and since params isn't the same object as base_gate.params (which ControlledGate implicitly expects), we get the problem.

That said, because Qiskit makes such heavy use of deepcopy (because our ownership model of gates isn't great), in practice this covers most of the times you might actually see that base_gate doesn't get updated. For example, QuantumCircuit.__eq__ implicitly deep copies both circuits because the equality is done via DAGCircuit (to resolve basic data-flow graph isomorphisms), and the conversion involves a copy. You can still see that there's some issues, though:

In [1]: from qiskit.circuit import Parameter, QuantumCircuit
   ...: a, b, c, d = (Parameter(x) for x in "abcd")
   ...:
   ...: qc1 = QuantumCircuit(2)
   ...: qc1.cu(0.1, 0.2, 0.3, 0.4, 0, 1)
   ...:
   ...: qc2 = QuantumCircuit(2)
   ...: qc2.cu(a, b, c, d, 0, 1)
   ...: qc2.assign_parameters([0.1, 0.2, 0.3, 0.4], inplace=True)
   ...: qc1 == qc2  # because there's a secret deepcopy
Out[1]: True

In [2]: qc1.data[0] == qc2.data[0]  # ... but _really_
Out[2]: False

In [3]: qc2.data[0].operation.params
Out[3]:
[ParameterExpression(0.1),
 ParameterExpression(0.2),
 ParameterExpression(0.3),
 ParameterExpression(0.4)]

In [4]: qc2.data[0].operation.base_gate.params
Out[4]: [Parameter(a), Parameter(b), Parameter(c)]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think some of the complexity may stem from the @setter for params being at the list level. QuantumCircuit._assign_parameter retrieves a reference to Instruction._params and modifies a single element directly. This process was causing the assignment to CUGate.params to be ineffective since the line return self.base_gate.params + self._params creates a new instance and the assignment is to this new copy (resulting in a no-op to the gate itself).

I included CUGate.__deepcopy__ since CUGate._params were no longer inherited from Instruction but managed at the CUGate level. Without that overload any QuantumCircuit.assign_parameters that was not inline was creating two instances of the CUGate in two different circuits that shared a single _params instance. The equivalence_library copies were also sharing a single list instance with the original gate as well.

An alternative was to add a setter method for _params that operated at the individual element level instead of the list level. I think this requires adding a ParameterList() class to manage the list. That seems like the first step in refactoring how parameters are managed; since next it seems logical to move the assign/bind logic out of QuantumCircuit and into that new class. But then the relationship between the ParameterList QuantumCircuit and Instruction didn't seem obvious to me. Not sure what the appetite for that level of refactoring is, but I can open a issue for that if it is something worth considering.

9 changes: 9 additions & 0 deletions releasenotes/notes/fix-cu-assign-parameters-fbf6f21d3e0c8c0b.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
fixes:
- |
Changed the :meth:`~qiskit.CUGate.params` property and setter to handle the additional parameter
gamma for the CUGate global_phase and to maintain the base gate, UGate, parameters
Added the :meth:`~qiskit.CUGate.__deepcopy__` to copy the CUGate parameters
Fixes `#7410 <https://github.com/Qiskit/qiskit-terra/issues/7410>`_.
Added CU3 to CU equivalence to the equivalence library
Fixes `#7326 <https://github.com/Qiskit/qiskit-terra/issues/7326>`_.
21 changes: 21 additions & 0 deletions test/python/circuit/test_parameters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1162,6 +1162,27 @@ def test_parametervector_resize(self):
self.assertIs(element, vec[1])
self.assertListEqual([param.name for param in vec], _paramvec_names("x", 3))

def test_assign_cugate(self):
"""Test assignment of CUGate
Check CUGate and UGate.control()"""

# CUGate
ps = [Parameter("t"), Parameter("p"), Parameter("l"), Parameter("g")]
qc = QuantumCircuit(2)
qc.cu(*ps, 0, 1)
assigned_qc = qc.assign_parameters(dict(zip(ps, [0.1, 0.2, 0.3, 0.4])), inplace=False)
self.assertFalse(assigned_qc.decompose().parameters)

# UGate.control()
theta = Parameter("t")
qc_c = QuantumCircuit(2)
qc_c.crx(theta, 1, 0)
custom_gate = qc_c.to_gate().control()
qc = QuantumCircuit(3)
qc.append(custom_gate, [0, 1, 2])
assigned_qc = qc.assign_parameters({theta: 0.5})
self.assertFalse(assigned_qc.decompose().parameters)

def test_raise_if_sub_unknown_parameters(self):
"""Verify we raise if asked to sub a parameter not in self."""
x = Parameter("x")
Expand Down