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

Fix indent & improve consistency of primitive docs with Qiskit #7897

Merged
merged 2 commits into from
Apr 7, 2022

Conversation

Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Apr 6, 2022

Summary

There was an indent missing in the EstimatorBase:

image

And not all functions were fully documented (missing input args or return docs). Also I tried to improve the consistency with the rest of Qiskit whre we usually use full sentences in the docs, but feel free to reject or adjust that!

Details and comments

Is there a reason why we use collections.abc for typehints instead of typing as in the most of the (or all?) codebase? 🙂

@Cryoris Cryoris added Changelog: None Do not include in changelog documentation labels Apr 6, 2022
@Cryoris Cryoris requested review from a team, ikkoham and t-imamichi as code owners April 6, 2022 11:50
@qiskit-bot
Copy link
Collaborator

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:

@mtreinish mtreinish added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Apr 6, 2022
@coveralls
Copy link

coveralls commented Apr 6, 2022

Pull Request Test Coverage Report for Build 2106039826

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.005%) to 83.927%

Files with Coverage Reduction New Missed Lines %
qiskit/pulse/library/waveform.py 3 89.36%
Totals Coverage Status
Change from base Build 2103378125: -0.005%
Covered Lines: 54136
Relevant Lines: 64504

💛 - Coveralls

@ikkoham
Copy link
Contributor

ikkoham commented Apr 6, 2022

Thank you so much and sorry for my mistakes. This is the right direction.
I just have one question, should we use Sphinx's :class:ClassName etc. in the doc string? For html it is obviously readable, but I am concerned that it may be difficult to read when viewed with the help function (or completion). I know that this is used in QuantumCircuit and others, but I would like to check it once.

Is there a reason why we use collections.abc for typehints instead of typing as in the most of the (or all?) codebase?

typing.Sequence is deprecated since Python 3.9, so if we use __future__.annotations , I believe that collections.abc is better.

@Cryoris
Copy link
Contributor Author

Cryoris commented Apr 6, 2022

just have one question, should we use Sphinx's :class:ClassName etc. in the doc string?

I don't think it's strictly necessary since most of the classes could be accessed via the type-documentation. But also I don't think there's harm in being precise and these paths are not very long. We can do which ever you prefer 🙂

Copy link
Contributor

@ikkoham ikkoham left a comment

Choose a reason for hiding this comment

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

@Cryoris Thank you. I agree that it is not very long. LGTM.

@Cryoris Cryoris merged commit e3c4e76 into Qiskit:main Apr 7, 2022
@Cryoris Cryoris deleted the update-primitive-docs branch April 7, 2022 07:49
mergify bot pushed a commit that referenced this pull request Apr 7, 2022
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit e3c4e76)
Cryoris added a commit that referenced this pull request Apr 7, 2022
)

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit e3c4e76)

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants