-
Notifications
You must be signed in to change notification settings - Fork 126
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
Updated the calibration docs #1241
base: main
Are you sure you want to change the base?
Conversation
@nitin-pandita Thanks for opening the PR—can you sign the CLA? |
Done |
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 a lot for taking this on and improving the docs of Qiskit Experiments! The changes look mostly good to me up to a naming convention for calibration experiments.
|
||
.. code-block:: python | ||
|
||
RoughFrequencyCal(BaseCalibrationExperiment, QubitSpectroscopy) | ||
class RoughFrequency(BaseCalibrationExperiment, QubitSpectroscopy): |
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 should have Cal
at the end. We use this as convention to indicate that it is a calibration experiment.
class RoughFrequency(BaseCalibrationExperiment, QubitSpectroscopy): | |
class RoughFrequencyCal(BaseCalibrationExperiment, QubitSpectroscopy): |
This ensures that the ``run`` method of :class:`.RoughFrequencyCal` will be the | ||
run method of the :class:`.BaseCalibrationExperiment` class. Furthermore, developers | ||
must explicitly call the :meth:`__init__` methods of both parent classes. | ||
This ensures that the `run` method of `RoughFrequency` will be 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.
This ensures that the `run` method of `RoughFrequency` will be the | |
This ensures that the `run` method of `RoughFrequencyCal` will be 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.
Sure I will fix that
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 for contributing. Note that Sphinx's rst notation is different from markup—one set of backticks makes the text italics instead of styled like code, so for most of the cases you want to use two sets of backticks. Please preview your docs by building locally or checking the artifacts produced during GitHub's CI runs.
an auto_update variable which, by default, is set to True. If this variable, | ||
is True then the run method of the experiment will call :meth:`~.ExperimentData.block_for_results` | ||
and update the calibrations instance once the backend has returned the data. | ||
instance of :class:`~qiskit_experiments.calibration_management.Calibrations`. Furthermore, calibration experiments also specify |
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 change is unneeded. The dot syntax in Sphinx links to the correct class with that name as long as it's unique. You can see for yourself in the current docs that Calibrations
is already correctly linked: https://qiskit.org/ecosystem/experiments/dev/stubs/qiskit_experiments.calibration_management.BaseCalibrationExperiment.html#qiskit_experiments.calibration_management.BaseCalibrationExperiment
is True then the run method of the experiment will call :meth:`~.ExperimentData.block_for_results` | ||
and update the calibrations instance once the backend has returned the data. | ||
instance of :class:`~qiskit_experiments.calibration_management.Calibrations`. Furthermore, calibration experiments also specify | ||
an `auto_update` variable which, by default, is set to True. If this variable |
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.
an `auto_update` variable which, by default, is set to True. If this variable | |
an ``auto_update`` variable which, by default, is set to True. If this variable |
and update the calibrations instance once the backend has returned the data. | ||
instance of :class:`~qiskit_experiments.calibration_management.Calibrations`. Furthermore, calibration experiments also specify | ||
an `auto_update` variable which, by default, is set to True. If this variable | ||
is True, then the `run` method of the experiment will call :meth:`~qiskit_experiments.base_experiment.ExperimentData.block_for_results` |
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 True, then the `run` method of the experiment will call :meth:`~qiskit_experiments.base_experiment.ExperimentData.block_for_results` | |
is True, then the :class:`~.BaseCalibrationExperiment.run` method of the experiment will call :meth:`~.ExperimentData.block_for_results` |
#. `_get_schedules_from_options` | ||
#. `_get_schedules_from_calibrations` | ||
#. `_get_schedules_from_defaults` |
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.
provides a default update methodology that subclasses can override if a more elaborate behavior | ||
is needed. At the minimum, the developer must set the variable `_updater` which | ||
should have an `update` method and can be chosen from the library | ||
:mod:`~qiskit_experiments.calibration_management.update_library`. See also |
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 not a module, just a file, so you need to update the language here. The Sphinx syntax in this paragraph also needs to be fixed.
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.
On it
is needed. At the minimum, the developer must set the variable `_updater` which | ||
should have an `update` method and can be chosen from the library | ||
:mod:`~qiskit_experiments.calibration_management.update_library`. See also | ||
:class:`~qiskit_experiments.calibration_management.update_library.BaseUpdater`. If no updater |
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 link won't work because BaseUpdater
isn't currently exposed in the API toctree. You need to add from .update_library import BaseUpdater
to calibration_management/__init__.py
.
Summary
Please describe what this PR changes as concisely as possible. Link to the issue(s) that this addresses, if any.
Details and comments
Some details that should be in this section include:
Note that this entire PR description field will be used as the commit message upon merge, so please keep it updated along with the PR. Secondary discussions, such as intermediate testing and bug statuses that do not affect the final PR, should be in the PR comments.
PR checklist (delete when all criteria are met)
CONTRIBUTING.md
.reno
if this change needs to be documented in the release notes.