-
Notifications
You must be signed in to change notification settings - Fork 82
Implement generic MultiControlledBloq using single-controlled subbloq + And ladder.
#1155
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
Implement generic MultiControlledBloq using single-controlled subbloq + And ladder.
#1155
Conversation
|
|
||
|
|
||
| @frozen | ||
| class MultiControlledBloq(Bloq): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see @mpharrigan's point that having both Controlled(sub_bloq, ctrl_spec) and MultiControlledBloq(sub_bloq, ctrl_spec) can be confusing and the implementation of both would actually be identical.
Regarding interface vs implementation, it may make things cleaner if we rename MultiControlledBloq to be more descriptive of the implementation, like ControlledViaAndLadder() and explain that the implementation performs CtrlSpecAnd -> Bloq.controlled() -> CtrlSpecAnd.adjoint() to decompose a bloq controlled by arbitrary CtrlSpec to a singly controlled Bloq via an And / And^{dagger} ladder and uses
Then, this specific implementation can be used by users who want to depend upon this behavior instead of the behavior getting potentially updated if we decide to update the decomposition of Controlled Bloq later.
MultiControlledPauli can get removed and we will simply update the call sites to use ControlledViaAndLadder(ctrl_spec, XGate())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the bloq name ControlledViaAndLadder, will use that
tanujkhattar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Left a final round of comments
| target = soqs.pop('target') | ||
| (target_reg_name,) = [reg.name for reg in self.target_bloq.signature] | ||
| if self.n_ctrls == 0: | ||
| # TODO discuss if we should remove support for this case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was specifically added after #677
tanujkhattar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
#1131
ControlledViaAndthat computes a single qubit control usingCtrlSpecAnd, and uses a single-controlled subbloq.MultiControlPaulito delegate to this. For the moment I did not remove it fully as it breaks too many things that switch between cirq-style and qualtran-style defs. This should be cleaned up in a follow-up PR.