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
Three qubit decomposition #2873
Conversation
significant changes since the review
@viathor - the last one in the Three Qubit Decomposition Series :D Thank you for all the reviews so far! |
return _optimize_multiplexed_angles_circuit(ops) | ||
|
||
|
||
def _multiplexed_angles(theta: Union[List[float], np.ndarray]) -> np.ndarray: |
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.
Every invocation of _multiplexed_angles
is followed by very similar code which builds the circuit in docstring below and calls an optimizer on it.
WDYT about moving the circuit construction into this function? This would give it a signature like
def _rotation_multiplexor_to_ops(
s0: cirq.Qid,
s1: cirq.Qid,
d2: cirq.Qid,
theta: Union[List[float], np.ndarray],
rotation: Callable[[float], cirq.Gate],
controlled_gate: cirq.Gate,
) -> Sequence[cirq.Operation]:
where s0 and s1 are the select qubits, d2 is the data qubit, rotation is a factory function that given an angle makes a rotation gate (e.g. cirq.ry
) and controlled_gate is a two-qubit controlled gate like CNOT or CZ.
I think this change would make the rest of the code read better, it would fit nicely with other multiplexor factories you defined and also has the nice property that the new function maps to a clear concept. It would also reduce the number of functions in this module since you'd be able to eliminate _cs_to_ops
and _middle_multiplexor_to_ops
.
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 think I'll push back on this one. I tried it and I'm not convinced - as while it might save some space, I'm not sure it will be more readable / understandable / testable. I really like the fact that the individual building blocks map to the concepts in the paper. I find that easier to follow. Also, the unit tests are thoroughly testing each primitive one by one. If I start to merge them further up, I'd have to write much more complicated tests.
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.
Fair enough.
Regarding tests: Unit tests that verify behavior of private help functions make refactoring harder. Ideally, tests would verify behavior of such helper functions via specially chosen input data. This is why many best practices docs (e.g. Google's own testing best practices) encourage testing public APIs.
@viathor - ready for a final review - I hope you are okay with my reasoning around the outstanding items. |
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 with a few minor comments.
Automerge cancelled: A status check is failing. |
Adds cirq.three_qubit_unitary_to_operations.
For the design doc, please see the attached 3qubit-decomposition.pdf
Fixes #451.