Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nitin-pandita
Copy link

@nitin-pandita nitin-pandita commented Aug 1, 2023

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:

  • Why this change was necessary
  • What alternative solutions were considered and why the current solution was chosen
  • What tests and documentation have been added/updated
  • What do users and developers need to know about this change

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)

  • I have read the contributing guide CONTRIBUTING.md.
  • I have added the tests to cover my changes.
  • I have updated the documentation accordingly.
  • I have added a release note file using reno if this change needs to be documented in the release notes.

@CLAassistant
Copy link

CLAassistant commented Aug 1, 2023

CLA assistant check
All committers have signed the CLA.

@coruscating
Copy link
Collaborator

@nitin-pandita Thanks for opening the PR—can you sign the CLA?

@nitin-pandita
Copy link
Author

@nitin-pandita Thanks for opening the PR—can you sign the CLA?

Done

Copy link
Contributor

@eggerdj eggerdj left a 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):
Copy link
Contributor

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.

Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This ensures that the `run` method of `RoughFrequency` will be the
This ensures that the `run` method of `RoughFrequencyCal` will be the

Copy link
Author

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

Copy link
Collaborator

@coruscating coruscating left a 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
Copy link
Collaborator

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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`

Comment on lines +80 to +82
#. `_get_schedules_from_options`
#. `_get_schedules_from_calibrations`
#. `_get_schedules_from_defaults`
Copy link
Collaborator

Choose a reason for hiding this comment

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

These methods and BaseCalibrationExperiment.get_schedules() were removed in #547, therefore this section needs to be updated as I highlighted in the issue. @eggerdj what do you think the text here should be? Should these two paragraphs simply be removed?

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
Copy link
Collaborator

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.

Copy link
Author

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants