-
Notifications
You must be signed in to change notification settings - Fork 70
Add unary iteration function to enable writing coherent for-loops #83
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
Conversation
this many what? |
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 looks good in principle. A few better variable names and docstrings could be a big help
cirq_qubitization/unary_iteration.py
Outdated
yield from self.decompose_single_control( | ||
and_target, selection, selection_ancillas, target, **extra_regs | ||
def unary_iteration_loops( | ||
index: int, indices: Dict[str, int], controls: Sequence[cirq.Qid] |
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.
some documentation would help here. index is the current selection index? i.e. how many deep we are in nested loops? why do we have indices
?
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.
Added documentation and used more descriptive variable names. PTAL.
cirq_qubitization/unary_iteration.py
Outdated
return | ||
anc, sq = ancilla[sl], selection[sl] | ||
ops.append(and_gate.And((1, 0)).on(control, sq, anc)) | ||
yield from _unary_iteration_segtree(ops, anc, selection, ancilla, sl + 1, l, m, L, R) |
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.
still still has tricky qubit aliasing #16 which I think will get us into hot water at some point in the future. I'm fine punting for now but I wanted to call attention to it again. Maybe having first-class allocation will make this easier since we won't have to do as much bookkeeping of ancillae
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.
We could always get rid of the sl
variable used for qubit aliasing and just use selection[0]
and ancilla[0]
(where ancilla[0]
can be replaced by qubit allocation / deallocation in the future). The recursive function call would then pass selection[1:]
and ancilla[1:]
instead of selection
, ancilla
-- but we'd need to use a smarter data structure to pass a sliced "View" of the underlying lists instead of copying them on each recursive function call (which would make it O(N ** 2)).
Is this what you want to do? I am not sure I understand the concerns here wrt "tricky qubit aliasing".
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.
yes, the slicing thing is what I had in mind. I guess it's true that if these registers are large the slicing cost (without views -- as you point out) could be significant.
For the record: I would be hyped if we didn't have to have n Qubit objects for each n-bitwidth register; see #84 . Consistent with your point: that PR has Split and Join operations that perform the role of "views"
…nto unary_iteration_loop
@mpharrigan Thanks for the detailed comments. This is ready for a re-review! |
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.
very cool
This PR makes it easier, faster and a lot more intuitive to write coherent for-loops using unary iteration. Specifically, it introduces the following changes:
unary_iteration
method, similar torange(L, R)
in python, which can be used to unary iteration circuits by iterating over theR - L
unique leaves of the unary iteration segment tree and enabling users to conditionally attach different operation sequences for each leaf.N
andM
using selection integersi
andj
required a unary iteration tree with2 ** ceil(log(NM))
leaf nodes -- this is because we were constructing a single large instead of multiple nested trees. Now, we first construct a tree with N leaves for indexi
; and then at each leaf of this tree we attach the tree for indexj
which hasM
leaves. Therefore, the combined tree has exactlyN * M
leaves. Note that the T-complexty of unary iteration (with a single control) is4 * (L - 1)
whereL
is the number of leaves. Thus, the T-complexity of this improved implementation is now optimal with regards to the original proposal. This can also be seen using the following test:cirq.X(ancilla[0]).controlled_by(selection[0], control_values=[0])
, acirq.ControlledOperation
used in https://github.com/ncrubin/cirq-qubitization/blob/19c6f9150ccd21ee8c3eef6d0d75bb6da57503ca/cirq_qubitization/unary_iteration.py#L176) with its decomposition into cliffords that the T-complexity protocol can understand (i.e.[X, CNOT, X])
). Right now, thet_complexity
protocol is not able to recognize that thecirq.ControlledOperation
is a clifford and therefore ends up addingrotations
count. cc @NoureldinYosri It would be good to brainstorm how we can take care of such cases automatically. I've also filed a related issue in Cirq. See Improve coverage of_decompose_into_clifford_with_qubits_
andhas_stabilizer_effect
protocols Cirq#5906