Skip to content

Conversation

@yaelbh
Copy link
Collaborator

@yaelbh yaelbh commented Jul 29, 2021

Summary

Addressing #254, mimicking #255.

Details and comments

Currently failing (at least locally, for me, let's see how the CI reacts) with:

reading sources... [ 69%] stubs/qiskit_experiments.library.characterization.ResonanceAnalysis                                                                                                                                                 
Exception occurred:
  File "/home/hrlquantum/work/experiments/NaokiDoc/docs/_ext/custom_styles/styles.py", line 251, in _extra_sections
    defaults=self._target_cls._default_transpile_options().__dict__,
AttributeError: type object 'ResonanceAnalysis' has no attribute '_default_transpile_options'

Please fix it (probably @eggerdj), then I guess that this PR will be ready to go.

@yaelbh
Copy link
Collaborator Author

yaelbh commented Jul 29, 2021

Yes, CI agrees with my local run

Copy link
Collaborator

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

Thanks @yaelbh . If you add template, this will be applied to all module under the autosummary. If you want to apply this partly, you can do something like
https://github.com/Qiskit/qiskit-experiments/blob/36fe3134e4f1ae9e3e2b5654d7169905ac191365/qiskit_experiments/library/__init__.py#L23-L35

Please let me know if you find any difficulty.

- :math:`offset`: Base line of the decay curve
- :math:`t1`: This is the fit parameter of main interest
#section: fit Parameters
defpar amplitude:
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
defpar amplitude:
defpar a:

This is written as a in the fit model. If you want to write some word here, you may want to add \rm, i.e. \rm amplitude, since this value will be shown in the math mode and become Itaric.

.. autosummary::
:toctree: ../stubs/
:template: autosummary/experiment.rst
Copy link
Collaborator

Choose a reason for hiding this comment

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

@yaelbh
Copy link
Collaborator Author

yaelbh commented Aug 1, 2021

@nkanazawa1989 Thanks for the review, I fixed according to your comments 😄

@yaelbh
Copy link
Collaborator Author

yaelbh commented Aug 1, 2021

  1. I have the same issue like Merav, of seeing #section.
  2. Why does it say that there are no experiment and analysis options, and how to fix it?

image

Copy link
Collaborator

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

Thanks @yaelbh. To write options, you need to provide documentation for analysis options and experiment options in self.__AnalysisClass__._default_options and self._default_experiment_options. Here you can find an example

https://github.com/Qiskit/qiskit-experiments/blob/30814c947e51b43cfac93738d0385314ba450cd5/docs/_ext/custom_styles/example/example_experiment.py#L105-L108

https://github.com/Qiskit/qiskit-experiments/blob/30814c947e51b43cfac93738d0385314ba450cd5/docs/_ext/custom_styles/example/example_experiment.py#L192-L195

These options are automatically parsed (if some options are already defined in the superclass the Sphinx extension will check superclass and port documentation from there, so you don't need to write documentation multiple times) and shown in the class documentation. This will be propagated to set_*_options method in future extension.

Calibration module has already completed so you can also check this :)


@classmethod
def _default_options(cls):
"""Default analysis options"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to list descriptions introduced in this subclass here. i.e.

Suggested change
"""Default analysis options"""
"""Default analysis options
Analysis Options:
t1_guess (Type): Doc
amplitude_guess (Type): Doc
offset_guess (Type): Doc
"""

@yaelbh
Copy link
Collaborator Author

yaelbh commented Aug 2, 2021

Current status:

  1. I still see "#section".
  2. In the T1 page there's "reference to analysis class: T1Analysis" without a link to the doc of this class.

@coruscating coruscating added this to the Release 0.2 milestone Aug 2, 2021
Copy link
Collaborator

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

LGTM thanks @yaelbh

@nkanazawa1989 nkanazawa1989 merged commit 5530e5a into qiskit-community:main Aug 3, 2021
paco-ri pushed a commit to paco-ri/qiskit-experiments that referenced this pull request Jul 11, 2022
* Adjust T1 doc strings to automatic template

* fixes to t1_analysis doc strings

* reverting previous changes to init files

* changes to init

* Update qiskit_experiments/library/characterization/t1_analysis.py

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

* Update qiskit_experiments/library/characterization/t1_analysis.py

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

* document t1 options

* black

* typo fix

* moved experiment options doc to the correct location

* black

* minor fix

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
Co-authored-by: knzwnao <knzwnao@jp.ibm.com>
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.

3 participants