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

Miscellaneous Doc Fixes #5754

Merged
merged 5 commits into from
Jul 13, 2022
Merged

Conversation

dstrain115
Copy link
Collaborator

Miscellaneous Doc Fixes

Adding module strings so that package doc pages show up better.

Adding a few doc strings.

Changing a two-line summary to one-line.

Changing FSimGate to remove "gate family" which has a particular meaning in cirq.

@dstrain115 dstrain115 requested review from wcourtney, a team, vtomole, cduck and verult as code owners July 13, 2022 15:50
@CirqBot CirqBot added the size: M 50< lines changed <250 label Jul 13, 2022
0 & 0 & 1 & 0 \\
0 & 0 & 0 & e^{i rads} \\
\end{bmatrix}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing closing $$

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Added.

such that: $U = Z^{\phi_0 / \pi} Y^{\phi_1 / \pi} Z^{\phi_2 / \pi}$.
for the Pauli matrices Y and Z. That is, phasing around Z by $\phi_0$ radians,
then rotating around Y by $\phi_1$ radians, and then phasing again by
$\phi_2$ radians will produce the same effect as the original unitary.
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is inconsistency here in the order the rotations are applied between the formula and the text.

(Recall that for functions A, B and C the composition U=ABC is the result of applying C first followed by B followed by A. Yeah. I know. I have a book about character theory that switched function application notation from f(x) to (x)f to "fix" the problem.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Changed the ordering and added a short note.

r"""Breaks down a 2x2 unitary into more useful ZYZ angle parameters.

Given a unitary U, this function returns three angles: $\phi_0, \phi_1, \phi_2$,
such that: $U = Z^{\phi_0 / \pi} Y^{\phi_1 / \pi} Z^{\phi_2 / \pi}$.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Remove dot since the sentence doesn't terminate after the formula.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@@ -71,7 +71,13 @@ def _rotation_matrix(angle: float) -> np.ndarray:


def deconstruct_single_qubit_matrix_into_angles(mat: np.ndarray) -> Tuple[float, float, float]:
"""Breaks down a 2x2 unitary into more useful ZYZ angle parameters.
r"""Breaks down a 2x2 unitary into more useful ZYZ angle parameters.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd remove "more useful". It's judgmental and incorrect. Matrix is more useful in some cases and angles are more useful in others (try computing the trace from the angles).

Suggestion:

Breaks down a 2x2 unitary into ZYZ rotation angles.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@@ -12,6 +12,8 @@
# See the License for the specific language governing permissions and
# limitations under the License.

"""Transformers for compiling to Google-specific Sycamore gate."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: I'd say "Google-specific gates" since we may have others in future (and we're unlikely to remember to come back to fix the comment).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to "Transformers for compiling to Google-specific gates, such as Sycamore" since Sycamore is currently the only transformer we have currently, but, as you point out, this could expand.

@dstrain115
Copy link
Collaborator Author

Thanks for the detailed review! PTAL

@dstrain115 dstrain115 requested a review from viathor July 13, 2022 17:32
Copy link
Collaborator

@viathor viathor left a comment

Choose a reason for hiding this comment

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

LGTM

@dstrain115 dstrain115 added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jul 13, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jul 13, 2022
@CirqBot CirqBot merged commit 38bed09 into quantumlib:master Jul 13, 2022
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Jul 13, 2022
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Miscellaneous Doc Fixes

Adding module strings so that package doc pages show up better.

Adding a few doc strings.

Changing a two-line summary to one-line.

Changing FSimGate to remove "gate family" which has a particular meaning in cirq.
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
Miscellaneous Doc Fixes

Adding module strings so that package doc pages show up better.

Adding a few doc strings.

Changing a two-line summary to one-line.

Changing FSimGate to remove "gate family" which has a particular meaning in cirq.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants