Fix CUGate parameter assignment issues

[rrodenbusch]

Modified CUGate.params, both property and setter, to handle all 4 params [theta,phi,lam,gamma] and maintain base_gate, CUGate, parameters [theta,phi,lam]
Added qiskit.CUGate.__deepcopy__ to maintain all 4 params in a copy
Added CU3 to CU equivalence to the equivalence library
Added test case test_assign_cugate to test_parameters.TestParameters
Fixes #7410
Fixes #7326

Summary

Maintain all 4 params [theta,phi,lam,gamma] in CUGate but only pass [theta,phi,lam] to the base_gate UGate. This allows assignment of parameters to CUGates with current assign_parameters methodology; and allows cu3_to_cu mapping back into the equivalence library.

Details and comments

Fixes #7410 Problem extracting Statevector when using a binded parametric controlled gate
Fixes #7362 CUGate's params getter does not comply with circuit's assign_parameters

[qiskit-bot]

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

• @Cryoris
• @Qiskit/terra-core
• @ajavadia

[jakelishman]

Thank you very much for looking at this. I'll be very glad to see the back of the CUGate problems. I saw a minor thing in the new parameters setter, and perhaps a more pressing thing in the overall direction, but that made me think a bit more: our problems here stem from the fact that UGate is not logically the base_gate of CUGate, because of the extra phase.

I wonder if it would make more sense overall if we stop dancing around and just define a full U4Gate to use as the base_gate of cu? @Cryoris, @ajavadia: perhaps you could weigh in on that?

[jakelishman]

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

[rrodenbusch]

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.

[jakelishman]

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)]

[rrodenbusch]

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.

[jakelishman]

@rrodenbusch: I'm really sorry for this PR staling - it's completely my fault. I just tried to move this to merge now, but unfortunately various bits and bobs have changed within the library since the time of this PR, and the workaround here was no longer sufficient. I made #11032 to be the spiritual successor of this PR, and since I built heavily on the research you'd done to find a viable strategy here, I've credited you as a co-author of it - I hope that's ok.

[rrodenbusch]

No worries. So am I to understand that you did find a fix?

…

On Tue, Oct 17, 2023, 17:02 Jake Lishman ***@***.***> wrote: @rrodenbusch <https://github.com/rrodenbusch>: I'm really sorry for this PR staling - it's completely my fault. I just tried to move this to merge now, but unfortunately various bits and bobs have changed within the library since the time of this PR, and the workaround here was no longer sufficient. I made #11032 <#11032> to be the spiritual successor of this PR, and since I built heavily on the research you'd done to find a viable strategy here, I've credited you as a co-author of it - I hope that's ok. — Reply to this email directly, view it on GitHub <#9118 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE3FKC4X2QQIEB4MX4Q2M5LX73573AVCNFSM6AAAAAAR45NND2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRXGI2TSOJTGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[jakelishman]

It solves several of the reported bugs, including the ones this PR was trying to fix, for sure, though the root cause is fundamentally that CUGate should not be a ControlledGate with a base_gate of UGate since they're incompatible. That's harder work to fix properly, though, and we're in the process of redoing several bits of how controlled-gate synthesis will work, so I'm not keen to add new dummy gates to be the CUGate.base_gate until we know if that'll be necessary.