Skip to content

Split the Transformers page into Transformers and Custom Transformers#5414

Merged
augustehirth merged 9 commits into
quantumlib:masterfrom
augustehirth:transformers_polish
Jun 13, 2022
Merged

Split the Transformers page into Transformers and Custom Transformers#5414
augustehirth merged 9 commits into
quantumlib:masterfrom
augustehirth:transformers_polish

Conversation

@augustehirth

@augustehirth augustehirth commented May 27, 2022

Copy link
Copy Markdown
Collaborator

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
Copy Markdown

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, cduck and vtomole as code owners May 27, 2022 21:36
@augustehirth

Copy link
Copy Markdown
Collaborator Author

CC @tanujkhattar

@@ -0,0 +1,491 @@
{

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Comment thread docs/transformers.ipynb
@@ -9,7 +9,7 @@
},

Copy link
Copy Markdown
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

Comment thread docs/transformers.ipynb
@@ -9,7 +9,7 @@
},

Copy link
Copy Markdown
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

Comment thread docs/transformers.ipynb
@@ -9,7 +9,7 @@
},

Copy link
Copy Markdown
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

@tanujkhattar tanujkhattar left a comment

Copy link
Copy Markdown
Collaborator

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.

3 participants