-
Notifications
You must be signed in to change notification settings - Fork 127
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
Refactored docs #983
Refactored docs #983
Conversation
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.
Overall it looks very good. I made a few small comments in some of the files. Here are a few more general comments:
- Why was T2Ramsey removed? I think a few other tutorials were not transferred to the guides, and I don't know if it was intentional.
- I suggest adding a section on ExperimentData, including Status.
- I suggest adding a section on AnalysisResult.
- The section on guides should have an introductory sentence (as do the
How to
andTutorials
. - In the
How-Tos
, you can remove theHow to
at the beginning of every section (because you have the topHow to
). - For the experiment guides, I know you simply transferred the tutorials to be guides. I think they should be reviewed. Two particular comments - for
Randomized Benchmarking
, add an example of setting thegate_error_rate
; InT2Hahn
the English should be improved.
docs/tutorials/custom_experiment.rst
Outdated
The FineAmplitude Experiment | ||
============================ | ||
|
||
The ``FineAmplitude`` calibration experiment repeats N times per gate with a pulse to amplify the under-/over-rotations in the gate to determine the optimal amplitude. |
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.
To my understanding FineAmplitude
is a characterization experiment, while the calibration version is FineAmplitudeCal
.
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.
@coruscating Please note this comment
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.
You're right, and I've updated the characterization experiment docstring so that it doesn't say calibration experiment anymore.
docs/tutorials/learning.rst
Outdated
There are two core types of composite experiments: | ||
|
||
* **Parallel experiments** run across qubits simultaneously. The circuits cannot overlap in | ||
qubits used. |
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.
run across qubits simultaneously
It sounds like the parallelization comes from some distributed algorithm. It's not clear that new circuits are constructed, that combine circuits of the sub-experiments.
The circuits cannot overlap in qubits used.
It sounds like circuits of each sub-experiment are not allowed to overlap between themselves.
I suggest not to mention batch experiments here. This allows to remove the itemization and provide a longer comprehensive paragraph explaining the doing of parallel experiments.
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've added more explanations and a figure, do you think it's clear enough now?
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.
The figure is great. The added sentence is still not clear, sorry :-). It still sound like sub-experiment circuits must have internally different qubits, and (until the figure) it's not clear what parallelized circuits
means.
docs/tutorials/getting_started.rst
Outdated
parallel_data = parallel_exp.run(backend, seed_simulator=101).block_for_results() | ||
|
||
Note that when options are set for a composite experiment, the child | ||
experiments's options are also set recursively. Let's examine how the parallel |
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 don't think it's always true. It depends on the option.
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, updated
from numpy.random import default_rng, Generator | ||
from qiskit import QuantumCircuit | ||
from qiskit_experiments.framework import BaseExperiment | ||
from qiskit.quantum_info import random_pauli_list |
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.
Consider arranging the imports, like pylint usually requests. Do we have pylint for the code snippets?
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.
Fixed these. I guess we can run a linter on the generated jupyter notebooks, but I don't know if we want to add that to the CI.
options.seed = None | ||
return options | ||
|
||
def circuits(self): |
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.
There's a subtle point that has to be clearly explained (perhaps it's already explained and I missed it). Naturally as a new-by user I'd expect the circuits to act on the physical qubits. Users have to understand that the circuit qubits are 0,..., len(physical_qubits)
, and a mapping is carried out later, in the transpilation.
This is also related to the explanation about parallel experiment, that says that the circuits are required to have different qubits. The accurate thing to say is that the sub-experiment must have different physical qubits.
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 point, added.
docs/tutorials/custom_experiment.rst
Outdated
expdata_noise = exp.run(noise_backend, shots=shots) | ||
counts_noise = expdata_noise.analysis_results("counts").value | ||
|
||
# Run noisy direct simulation of original circuit without randomization |
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.
What do you mean by "direct simulation"?
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.
It means simulating the original circuit without going through the experiment. I reworded so it should be less confusing now.
docs/tutorials/custom_experiment.rst
Outdated
Now we'll use what we've learned so far to make a full custom experiment from | ||
the :class:`.BaseExperiment` template. | ||
|
||
A randomized measurement experiment |
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.
Please revise the code of the experiment and analysis classes, in addition to my comments. It doesn't have the form of an example code that helps learn how to write an experiment.
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've started to break up the code with explanations in-between in the newest commit. What do you mean by this doesn't have the form of an example code? Do you think a dummy template experiment with the attributes, something like
class CustomExperiment(BaseExperiment):
def __init__(self, physical_qubits=None, dummy_option=None, backend=None):
"""Initialize the experiment."""
super().__init__(physical_qubits, analysis=CustomAnalysis(),backend=backend)
self.set_experiment_options(
dummy_option=dummy_option
)
@classmethod
def _default_experiment_options(cls) -> Options:
options = super()._default_experiment_options()
options.update_options(
dummy_option=None,
)
return options
...
would be helpful before introducing this experiment?
docs/tutorials/getting_started.rst
Outdated
|
||
Qiskit Experiments is built on top of Qiskit, so we recommend that you first install | ||
Qiskit following its :external+qiskit:doc:`installation guide <getting_started>`. | ||
Qiskit Experiments supports the same platforms and Python versions (currently 3.7+) as |
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.
Will we be aware about updating the Python version number here?
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.
Yes, PRs to update Python version in the future should update this string. It would be nice to pull it automatically from setup.py
but that will be work for another PR.
@@ -43,7 +42,8 @@ class T1(BaseExperiment): | |||
by fitting to an exponential curve. | |||
|
|||
# section: analysis_ref | |||
:py:class:`T1Analysis` | |||
:py:class:`.T1Analysis` |
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.
What does the dot in the beginning mean?
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.
There is a description here at the bottom of the section. Whether the dot is good here or not, I am not sure. T1Analysis is imported here so a search with the module name prepended should find it, but the real definition of T1Analysis is in a different module.
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 have not tested, but using the .
when there is a reference in scope might help suppress warnings like this one that I see in the output from the docs build:
/home/runner/work/qiskit-experiments/qiskit-experiments/docs/tutorials/getting_started.rst:100: WARNING: more than one target found for cross-reference 'run': qiskit_experiments.calibration_management.BaseCalibrationExperiment.run, qiskit_experiments.curve_analysis.BaseCurveAnalysis.run, qiskit_experiments.curve_analysis.BlochTrajectoryAnalysis.run
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 added this particular dot on the off chance it fixed all the T1 warnings (it did not), but it's optional here. In general, if the object name is unique, using dot then the name is much shorter than writing out the full path. A common problem in our docs are broken cross references because the wrong path is used or people tried to reference an object in a different path without the dot, so I think it's good to get into the habit of writing it this way. One caveat with the dot is when there are multiple objects with the same name, as shown in @wshanks's example, where you should use something like :meth:`.BaseCurveAnalysis.run`
to disambiguate instead if you're trying to reference the object that's not your local one.
Could we try to eliminate as many of the warnings and errors from sphinx as we can here? I think the
Could we do something about all the occurrences of The docs build okay with the warnings, but they are off-putting for people new to building the docs and can be misleading if someone adds documentation and thinks they might have been the cause of the warnings. |
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.
Here are a couple initial comments.
@wshanks I've been looking into these errors but I'm still unsure what's causing them. I compared one of the experiment classes that has build errors (and actually doesn't build for me at all locally, though the CI build works) with one that doesn't, but I couldn't seen anything different.
These go away if I'm sure this is a solvable problem since other docs don't have that gap but I'm not sure yet where the problem lies. |
@wshanks I've fixed the excess whitespace issue, but the build warnings seems to be a known mac-only local issue with no obvious fix: sphinx-doc/sphinx#10943 |
One thing I noticed -- some of the tutorials / guides dump files into the top level of the repo:
Could we get these files written to a temporary directory that gets cleaned up? Or at least a directory under docs/ that is in |
Also updated the navigation to link to individual tutorials and updated the composite experiment figure to be more readable.
…iments into refactored-docs
updated logic so that the attribute section is only rendered if there are valid elements
Linux builds don't like to see class variables, and they're already documented via the docstring so we can just skip them
Because of issues on mac, `tox -edocs` doesn't have multiprocessing anymore. the `docs-parallel` env has been added for CI and Linux builders. `docsnorst` renamed to `docs-minimal`. The class template for methods has been updated like attributes so the header doesn't render unless there are valid members.
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.
Here are some review comments on the new content.
meas_nc = len(self._measured_qubits) | ||
|
||
# Classical bits of the circuit | ||
circ_clbits = list(range(circ_nc)) |
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.
Maybe somewhere you want to mention that the input circuit could mid-circuit measurements? Or maybe for the sake of the example it is cleaner to assume no extra measurements and the measurements added by the experiment are the only measurements?
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 added a simple mid-circuit measurement circuit at the end to demonstrate this, though I don't run it. Maybe there's a more interesting example to actually run?
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.
Looks good to me!
### Summary This PR is essentially the documentation fixes from #1080 since the lint CI pipeline needs more work. ### Details and comments - The docs are no longer incompatible with the newest sphinx-autodoc-typehints so the version pin from #1017 has been removed. This closes #1018. - Updates curve analysis tutorial to the newest version (#983 had accidentally reverted to an older version) - Changes arxiv autolinks from pdfs to abstract page to be more user friendly - Renames the new ExperimentData how-to to rerunning analysis and adds additional info on using `add_data` - Moved functions from `curve_analysis/data_processing.py` to `utils.py` - Changes benchmarking experiments to verification experiments on the manual page to match the library API page - Added referenced classes and functions that weren't included in the docs - Various formatting fixes for passing Sphinx nitpick mode
Refactoring existing docs into four new areas: tutorials, how-tos, experiment guides, and the API reference. With contributions from @chriseclectic, @wshanks, @jaleipekoglu, and @spencerking.
Main changes:
See Also
section in docstrings to render the rst directly instead of parsing modulessphinx-panels
withsphinx-design
qiskit-sphinx-theme
to >=1.10qiskit-ibmq-provider
in docs and linked to migration guidetox -edocs-parallel
for a parallel build and renamed no jupyter-execute command totox -edocs-minimal