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

Split the Transformers page into Transformers and Custom Transformers #5414

Merged
merged 9 commits into from
Jun 13, 2022

Conversation

augustehirth
Copy link
Collaborator

@augustehirth augustehirth commented May 27, 2022

The existing Transformers page was pretty long, and this seemed like a suitable place to split it. Also added changed some phrasing, added some additional explanations/comments, and changed a couple examples. Also ran a formatter.

@CirqBot CirqBot added the size: L 250< lines changed <1000 label May 27, 2022
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@augustehirth augustehirth marked this pull request as ready for review May 27, 2022 21:36
@augustehirth augustehirth requested review from a team, vtomole and cduck as code owners May 27, 2022 21:36
@augustehirth
Copy link
Collaborator Author

CC @tanujkhattar

@@ -0,0 +1,491 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Update the links in this header to point to custom_transformers.ipynb instead of transformers.ipynb.


Reply via ReviewNB

@@ -0,0 +1,491 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Replace "Cirq provides primitive and decomposition utilities" with "Cirq provides circuit compilation primitives and gate decomposition utilities"


Reply via ReviewNB

@@ -0,0 +1,491 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:

"Primitives don't necessarily support everything that could go into a cirq.TransformContext object"

The reason we don't directly take a context object in primitives is:

a) We want users to be able to call the transformer primitives without necessarily creating a transformer that expects a transformer context (for example, if the transformation that they want to do can be expressed in just a single call to cirq.map_operations(circuit, custom_map_func).

b) Transformer primitives don't support automated logging, like the cirq transformers do.

Can we reword the sentence to better reflect this?


Reply via ReviewNB

@@ -9,7 +9,7 @@
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: "page covers presents the" remove covers or presents.


Reply via ReviewNB

@@ -9,7 +9,7 @@
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: "which composes a couple of the available" --> use few instead of couple, since we are composing more than 2 transformers.


Reply via ReviewNB

@@ -9,7 +9,7 @@
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: cirq.CircuitOperation instead of cirq.CircuitOperations

nit:

Replace "one of those operations was not very impactful" with "one of the merged connected component was equivalent to the identity operation"

nit:

Replace "without cirq.CircuitOperations and with terminal measurements." with "expanding intermediate cirq.CircuitOperations and aligning measurements to be terminal measurements".


Reply via ReviewNB

Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

LGTM % nits

@augustehirth augustehirth merged commit aa64d1c into quantumlib:master Jun 13, 2022
@augustehirth augustehirth deleted the transformers_polish branch June 13, 2022 17:05
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
…quantumlib#5414)

The existing Transformers page was pretty long, and this seemed like a suitable place to split it. Also added changed some phrasing, added some additional explanations/comments, and changed a couple examples. Also ran a formatter.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: L 250< lines changed <1000
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants