-
Notifications
You must be signed in to change notification settings - Fork 53
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
Gate decomposition via Householder reflections #758
base: main
Are you sure you want to change the base?
Conversation
…gging for a single one
…gging for a single one
Update with upstream
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.
Great contribution! @tanujkhattar can you take a look as well.
My major ask is: can we rename these bloqs and the module to something other than "decompose". We already use that term to mean something else. I see the reference uses the word "synthesis" whose verb would be synthesize
"""Extra soquets inside soqs are: | ||
* phase_grad: a phase gradient state of size phase_bitsize if internal_phase_gradient | ||
is set to False | ||
* refl_ancilla: a clean qubit in |0> if internal_refl_ancilla is set to False | ||
""" |
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 can probably just be line comments #
instead of a docstring. If you want this to be a docstring, it should follow the docstring format.
By "extra soquets" you mean "optional soquets"?
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.
Not really optional, but required in some situations. For example, if an user sets the internal_phase_grad
flag to False
then that register has to be provided, but it is not optional in that 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.
optional in a cosmic sense where you can set the flag to true
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.
@Vicara12 Thanks a lot for opening this PR and apologies for the late review!
I've left a round of comments - Would you still have time to address the comments and get this PR merged? Please let us know one way or the other since this would be a very useful primitive to get in.
Main suggestions are around making changes so we can support symbolic analysis.
return Signature.build( | ||
refl_ancilla=(not self.internal_refl_ancilla), | ||
gate_input=self.gate_bitsize, | ||
phase_grad=(not self.internal_phase_grad) * self.phase_bitsize, |
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.
Use QFxp
type for the phase gradient register
def signature(self) -> Signature: | ||
return Signature.build(target_reg=self.state_bitsize + 1, phase_grad=self.phase_bitsize) | ||
|
||
def build_composite_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.
Please implement a build_call_graph
for symbolic analysis.
Co-authored-by: Tanuj Khattar <[email protected]>
This code implements the first two points in this comment from issue #596 . It adds a bloq that decomposes an arbitrary unitary gate into householder reflections. It allows for partial specification of the gate, in which case the T-gate cost of the bloq is linearly proportional to the number of gates specified.
This code is a necessary part for the goal of implementing a MPS preparation algorithm, even though it is useful on its own for other applications.