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

Add base-gate callback to CUGate.params return #11032

Merged
merged 2 commits into from
Oct 19, 2023

Conversation

jakelishman
Copy link
Member

@jakelishman jakelishman commented Oct 17, 2023

Summary

This causes item-setting in CUGate.params to forward those sets on to the UGate in its base_gate unless they are setting the gamma parameter, which is not shared.

CUGate breaks several assumptions and components of the ControlledGate interface, including by having more parameters than its base_gate; ControlledGate assumes that its subclasses will not have a separate _params field, but instead will always just view onto their base_gate._params. This commit changes the behaviour of the params getter to create a temporary list that backreferences to the base gate, satisfying the requirement.

Details and comments

This is a replacement PR for #9118, as enough of Terra had moved on since that PR that the workaround in it was no longer sufficient for most test cases. This commit builds on the work from that PR and I've credited @rrodenbusch as a co-author on it, since they solved the initial problem, and it's just my fault that the PR then staled due to lack of review.

This commit is by no means ideal, but the root problem is that CUGate simply does not follow the ControlledGate interface; it does not satisfy $C[U] = \bigl(I - \lvert\psi\rangle\langle\psi\rvert\bigr)I + \lvert\psi\rangle\langle\psi\rvert U$ where $\lvert\psi\rangle$ is ctrl_state and $U$ is base_gate. Unfortunately, the class is probably a bit too heavily used for it to be easy to make the existing CUGate a new CU4Gate or so that doesn't subclass ControlledGate (although making a special _U4Gate that includes a global phase in its definition to use as the base_gate is a possibility).

Fix #7326
Fix #7410
Fix #9627
Fix #10002
Fix #10131

Close #9118 (replaces it).

@jakelishman jakelishman added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Oct 17, 2023
@jakelishman jakelishman requested a review from a team as a code owner October 17, 2023 21:55
@qiskit-bot
Copy link
Collaborator

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

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

This causes item-setting in `CUGate.params` to forward those sets on to
the `UGate` in its `base_gate` unless they are setting the `gamma`
parameter, which is not shared.

`CUGate` breaks several assumptions and components of the
`ControlledGate` interface, including by having more parameters than its
`base_gate`; `ControlledGate` assumes that its subclasses will not have
a separate `_params` field, but instead will always just view onto their
`base_gate._params`.  This commit changes the behaviour of the `params`
getter to create a temporary list that backreferences to the base gate,
satisfying the requirement.

Co-authored-by: Richard Rodenbusch <rrodenbusch@gmail.com>
@mtreinish mtreinish self-assigned this Oct 17, 2023
@mtreinish mtreinish added this to the 0.45.0 milestone Oct 17, 2023
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

So I'm fine with this fix, it's a clever albeit ugly way to thread the needle between the weird custom behavior of CUGate.params and also make it compatible with the normal object model for gates. I left one inline question about an implementation detail for python's list. But, I'm wondering if it would be better to just rip the bandaid off here and make CUGate conform the normal ControlledGate structure and remove the weird shared params list between itself and the base gate. It would probably be disruptive for existing users, but we do have a justification in that it really is breaking the object model and is different than every other controlled gate.

qiskit/circuit/library/standard_gates/u.py Show resolved Hide resolved
qiskit/circuit/library/standard_gates/u.py Outdated Show resolved Hide resolved
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

I think this is fine as a stop gap to fix the issues. I'm no super thrilled with the implementation here, but since it's all private we can revisit this and maybe any larger changes to CUGate for 1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment