Skip to content

Conversation

@MichaelBroughton
Copy link
Collaborator

@MichaelBroughton MichaelBroughton requested review from a team, cduck and vtomole as code owners May 23, 2022 23:05
@MichaelBroughton MichaelBroughton requested a review from viathor May 23, 2022 23:05
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@CirqBot CirqBot added the size: L 250< lines changed <1000 label May 23, 2022
@augustehirth augustehirth self-assigned this May 24, 2022
@@ -0,0 +1,357 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe add that the following only throws one exception?


Reply via ReviewNB

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is saying that a circuit with multiple exception-throwing issues (eg unsupported qubit and gate simultaneously), will only throw one exception, it might be good to show this in an example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think just showing the single exceptions on their own here is fine, unless we feel very strongly here ?

@dabacon
Copy link
Collaborator

dabacon commented May 24, 2022

Couple of drive by comments this morning.

},
"outputs": [],
"source": [
"qubit_set = metadata.qubit_set\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be worth explaining what these fields are in bullet points below.

Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI: I would still add more about the various attributes in the metadata portion of the class.

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. Added more descriptions and detail.

"source": [
"## 3. The `cirq.Device` interface\n",
"\n",
"For advanced users (such as vendors) it is also possible to implement your own Device with its own unique constraints and metadata information. Below you can implement our own fictituous device:"
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be good to put some more information here about what to do in order to implement your own device.
For instance, "The minimum you will need to do is write an init to initialize the object and a validate_operation function to validate a single operation. However, this interface supports constraints like not allowing adjacent operations with validate_moment, or even circuit level constrains like a maximum depth using validate_circuit. You can also add metadata to your object by subclassing DeviceMetadata." (etc)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with this. Details of the function types of init and validate_operation, as well as the types of data that can be stored in DeviceMetadata would be great to have in this doc, so people don't have to go to the reference if they're trying to match this template.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Expanded the wording as also added metadata example for the custom case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The docstrings for the below Device are looking great. I think I would still expand this paragraph to give some motivation as to the following class. For instance,
"Suppose we have a 5 qubit device that supports X/Y/Z single qubit and CZ gates between neighbors with a maximum of 10 CZ gates. We would then create an instance of the Device class that has a validate_operation to validate the gate types and adjacency requirements, and implement a validate_circuit to validate the maximum number of CZs."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added this sentence in.

" cirq.XPowGate, cirq.YPowGate, cirq.ZPowGate, cirq.CZPowGate\n",
" )\n",
"\n",
" def validate_operation(self, operation):\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is supposed to be an instructive example, I would put good docstrings in here, such as "this validate_operation does blah, but you can customize to support..." and really make use of the docstrings to help explain how to construct your own device and what the best practices are. (For instance, in validate_circuit, you can explain that it is only needed if their are circuit-level constraints.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added docstrings explaining behaviors in more detail

"id": "glMmeLevElt-"
},
"source": [
"The Sycamore device is a 2d grid device that exposes a `cirq.GridDeviceMetadata` with a uniform set of gates across all the qubits as well as a connectivity graph. You can explore the properties below:"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this needs to be reworded, since it is squishing two concepts together:
Sycamore device is a 2d grid device with a uniform set of gates across all the qubits.
It exposes a connectivity graph.

(Also, I think the connectivity graph is part of DeviceMetadata, not necessarily GridDeviceMetadata. You might want to make that clear, as well as explain that the GridDeviceMetadata contains additional information, such as gate durations and gateset).

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 call, added a few more cells to showcase the type hierarchy, was also more careful in how I reworded this section to explain the differences between DeviceMetadata and GridDeviceMetadata.

Copy link
Collaborator

@augustehirth augustehirth left a comment

Choose a reason for hiding this comment

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

LGTM mod typo

"id": "glMmeLevElt-"
},
"source": [
"The Sycamore device is a 2d grid device that exposes a `cirq.GridDeviceMetadata` with a uniform set of gates across all the qubits as well as a planar neearest neighbor connectivity graph. You can explore the properties below:"
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: "neearest"->"nearest"

@MichaelBroughton MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jun 14, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jun 14, 2022
@CirqBot CirqBot merged commit 40c33d3 into quantumlib:master Jun 14, 2022
@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jun 14, 2022
@CirqBot CirqBot removed the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jun 14, 2022
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
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.

5 participants