-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Added how-to guides #8624
Added how-to guides #8624
Conversation
Thank you for opening a new pull request. 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. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 5518892140
💛 - Coveralls |
Had to remove I explicitly set the |
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 just quickly skimmed things and left two small inline comments but they apply to all the docs being added here
Co-authored-by: Junye Huang <[email protected]>
Co-authored-by: Junye Huang <[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.
Review for compose_quantum_circuits.rst
Co-authored-by: Junye Huang <[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 looks great! I left a bunch of comments that are mostly just english corrections. I'm not a professional linguist so i don't really know how to explain my suggestions other than "it just sounds more fluent" as a native english speaker 😅 but if anything is confusing please lmk and I can try to explain
docs/how_to/use_sampler.rst
Outdated
0 1 | ||
|
||
The main difference from the previous case is that now you need to include the parameter values | ||
for which you want to evaluate the expectation value as a ``list`` of ``list``\ s of ``float``\ s. |
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.
is this paragraph correct? Shouldn't be talking about sampling probabilities not expectation values?
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.
Good catch! Fixed in ec0c28c
Co-authored-by: Abby Mitchell <[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.
Address review comments.
docs/how_to/use_sampler.rst
Outdated
0 1 | ||
|
||
The main difference from the previous case is that now you need to include the parameter values | ||
for which you want to evaluate the expectation value as a ``list`` of ``list``\ s of ``float``\ s. |
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.
Good catch! Fixed in ec0c28c
.. note:: | ||
In Qiskit Terra :math:`\leq 0.7`, the default behavior for the :func:`~.circuit_drawer` function is to use the ``'latex'`` output backend, and in :math:`0.6.x` that includes a fallback to ``'mpl'`` if ``'latex'`` fails for any reason. Starting with release :math:`> 0.7`, the default changes to the ``'text'`` output. |
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 think this information is not needed and extra information makes the page longer and distracting. Remove in be801cf
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.
Looking really good! Just a few last minor comments :)
|
||
In order to join two circuits with :meth:`~.QuantumCircuit.compose`, you only have to specify the circuit you want to insert. That way the qubits and bits of the smaller circuit will be included into the first qubits and bits of the bigger one in the original order they had. | ||
|
||
By default, :meth:`~.QuantumCircuit.compose` does not modify the original circuit to which it is applied but returns a new joint circuit object. This can be changed by setting the ``inplace`` argument to ``True``. |
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 think it would be good to put this sentence in a note box or a in brackets, as the following code example doesn't actually show how to use the inplace argument
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.
alternatively it might be nice to add a short code snippet showing the use of the inplace arg
|
||
In order to join two circuits with :meth:`~.QuantumCircuit.append`, you need to specify the circuit you want to add, as well as the qubits and classical bits (if there are any) onto which you want the circuit to be applied. | ||
|
||
Different from :meth:`~.QuantumCircuit.compose`, this method modifies the circuit it is applied to, instead of returning a new circuit. |
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.
Different from :meth:`~.QuantumCircuit.compose`, this method modifies the circuit it is applied to, instead of returning a new circuit. | |
Unlike :meth:`~.QuantumCircuit.compose`, this method modifies the circuit it is applied to, instead of returning a new circuit. |
└───────┘ | ||
|
||
|
||
Another difference between :meth:`~.QuantumCircuit.bind_parameters` and :meth:`~.QuantumCircuit.assign_parameters` is that for the latter, you can make it change your original circuit instead of creating a new one by setting the ``inplace`` argument to ``True``. |
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 in place argument feels similar to the one in the previous how to guide, I think it would be good to use the same format for the explanation in both.
|
||
.. testcode:: | ||
|
||
# Resulting quantum circuits are different if the quantum or classical registers have different relative 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.
nice comparison!
Using the :func:`~.circuit_drawer` function | ||
------------------------------------------- | ||
|
||
If you prefer to use a self-contained function instead of a :class:`~.QuantumCircuit` method to draw your circuit, you can do it with :func:`~.circuit_drawer` from :mod:`qiskit.visualization`. It has the exact same behavior as the :meth:`~.QuantumCircuit.draw` method above, except that it requires the circuit to be included as an argument. |
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.
Is there a reason a user might want to use the circuit_drawer
function at all? If there is a plausible scenario where this would be the preferred way maybe we should mention it?
|
||
.. _create circuit with qubit numbers: | ||
|
||
Create by specifying the number of qubits and classical bits |
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 feels a bit weird grammatically. I'd suggest saying something like "Create a circuit by specifying..."
|
||
q_2: | ||
|
||
Create from quantum and classical registers |
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.
same as above, just using "create" on it's own like this feels a bit odd grammatically. I think using something like "create a circuit from..." is a bit clearer
Since this type of content has moved over to github.com/Qiskit/documentation in the intervening yeras, I'll close this PR as staled now. Thanks for all the work that went into this - I'm sure it at a minimum informed the guides that got written in the separate documentation. If there's more to discuss, or components of this PR should be taken to the main documentation repository, please feel free to take the discussion there. |
Summary
Expanded the how-to guide section created in #9716.
Details and comments
This PR adds 4 guides written in
.rst
format:Note: This PR was originally meant to add the how-to section to
qiskit-terra
. However, several months later I added two guides about the primitives to this PR and later it was decided to move the primitive guides to their own PR (#9716) and give them higher priority. Therefore now this PR is more like an expansion of #9716.