-
Notifications
You must be signed in to change notification settings - Fork 79
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
Improve documentation on Registers and Qubits #1601
base: main
Are you sure you want to change the base?
Improve documentation on Registers and Qubits #1601
Conversation
Thanks for contributing to Qiskit documentation! Before your PR can be merged, it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. Thanks! 🙌 One or more of the following people are relevant to this code: |
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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.
Thanks so much, this is a lovely contribution! I just left a couple of minor comments but other than that its looking really good
"id": "6e7b212f", | ||
"metadata": {}, | ||
"source": [ | ||
"The index and register of a qubit can be obtained using the circuit's [`find_bit`](/api/qiskit/qiskit.circuit.QuantumCircuit#qiskit.circuit.QuantumCircuit.find_bit) method and its attributes. " |
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 believe in the original issue attached to this PR it was mentioned that a user was especially interested in the qc.qubits.index(your_qubit)
method of getting information. could you include something about this as well as the find_bit
method?
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 didn't include that due to this comment in a previous PR which says it's not recommended due to the significant performance overhead. Please let me know whether to include it or not in reference to this, thanks!
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 agree with @shraddha-aangiras. The two approaches have the same output, but find_bit
is more efficient, so I don't think we should suggest using index
.
@shraddha-aangiras we've recently edited the history on # Make sure you're on your branch
git switch improvedocs-issue1469
# Overwrite your branch with our history
git reset --hard origin/main
# re-apply your commits on top of the new history
git cherry-pick c370ad180de77c0b816dc025dd719806112f753a 361ac3ffb84190f25930a9c6a94819f4eb058c9f
# Check everything looks correct
git diff main
# If all good, force-push to your branch
git push --force Let me know if you have any questions. Sorry for the inconvenience. |
@frankharkins No problem, will get that along with the suggested changes (thanks @javabster!) fixed by tomorrow |
361ac3f
to
a976336
Compare
@frankharkins can you let me know if I did it right? Thanks! |
967327f
to
2172653
Compare
"id": "6e7b212f", | ||
"metadata": {}, | ||
"source": [ | ||
"The index and register of a qubit can be obtained using the circuit's [`find_bit`](/api/qiskit/qiskit.circuit.QuantumCircuit#qiskit.circuit.QuantumCircuit.find_bit) method and its attributes. " |
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 agree with @shraddha-aangiras. The two approaches have the same output, but find_bit
is more efficient, so I don't think we should suggest using index
.
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.
@shraddha-aangiras looks great! I have a few suggestions.
Also, sorry about my instructions, but glad you worked it out. (For others reading: git reset --hard origin/main
only works if "origin" refers to this repo, which is probably not the case for external contributors.)
Co-authored-by: Frank Harkins <[email protected]>
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.
This is a great contribution thank you! This satisfies what I was thinking of when I opened #1601
The only thing I'd ask before approving is if you could run the notebook linter to make sure all of the formatting is consistent. You'll just need to run
tox -e fix -- docs/**/*.ipynb
if you have tox installed.
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.
This is great! Thank you so much for your contribution!
docs/guides/construct-circuits.ipynb
Outdated
@@ -23,7 +23,7 @@ | |||
"source": [ | |||
"## What is a quantum circuit?\n", | |||
"\n", | |||
"A simple quantum circuit is a collection of qubits and a list of instructions that act on those qubits. To demonstrate, the following cell creates a new circuit with two new qubits, then displays the circuit's [`qubits`](/api/qiskit/qiskit.circuit.QuantumCircuit#qubits) attribute." | |||
"A simple quantum circuit is a collection of qubits and a list of instructions that act on those qubits. To demonstrate, the following cell creates a new circuit with two new qubits, then displays the circuit's [`qubits`](/api/qiskit/qiskit.circuit.QuantumCircuit#qubits) attribute, which is a list of [`Qubits`](/api/qiskit/circuit#qiskit.circuit.Qubit) in order." |
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.
In what order? The order in which they were defined?
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.
Clarified this in the latest commit, thank you!
Co-authored-by: Rebecca Dimock <[email protected]>
…qiskit-documentation into improvedocs-issue1469
"id": "87f1e053", | ||
"metadata": {}, | ||
"source": [ | ||
"## Measure qubits\n", |
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.
"## Measure qubits\n", | |
"<span id=\"measure-qubits\"></span>\n", | |
"## Measure qubits\n", |
@@ -23,7 +23,7 @@ | |||
"source": [ |
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 would suggest changing this first couple sentences to:
Measurements are used to sample the states of individual qubits and transfer the results to a classical register. Note that if you are submitting circuits to a Sampler primitive, measurements are required. However, circuits submitted to an Estimator primitive must not contain measurements.
Reply via ReviewNB
Based on user feedback from the Qiskit Slack, I added more explanations of commonly used methods and attributes of circuits. In particular:
fixes Better documentation on Registers and Qubits #1469