Skip to content

Improve QkCircuit C API docs #14259

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
May 12, 2025
Merged

Improve QkCircuit C API docs #14259

merged 11 commits into from
May 12, 2025

Conversation

mtreinish
Copy link
Member

@mtreinish mtreinish commented Apr 28, 2025 •
edited
Loading

Summary

This commit adds some more details to the C API docs for the new QkCircuit struct which was added in #14006.

Details and comments

TODO:

  • Document members of QkCircuitInstruction (they're inline docstrings, but those docstrings seem to be getting lost in the
    Rust -> C Header -> Breathe -> Sphinx path
  • Expand top level docs on the circuit struct to contain more details on how it works.

@mtreinish
Improve QkCircuit C API docs
1610c1a 
This commit adds some more details to the C API docs for the new
QkCircuit struct which was added in Qiskit#14006.
@mtreinish mtreinish added on hold Can not fix yet documentation Something is not clear or an error documentation priority: high Changelog: None Do not include in changelog labels Apr 28, 2025
@mtreinish mtreinish added this to the 2.1.0 milestone Apr 28, 2025
@mtreinish mtreinish requested a review from a team as a code owner April 28, 2025 21:03
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

@coveralls
Copy link

coveralls commented Apr 28, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 14968441393

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 4 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.01%) to 88.732%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 1 92.23%
crates/transpiler/src/passes/unitary_synthesis.rs 1 94.85%
crates/circuit/src/lib.rs 2 94.39%
Totals Coverage Status
Change from base Build 14931971977: 0.01%
Covered Lines: 76064
Relevant Lines: 85723
💛 - Coveralls

Cryoris
Cryoris reviewed Apr 29, 2025
View reviewed changes
@mtreinish @Cryoris
Apply suggestions from code review
421a538 
Co-authored-by: Julien Gacon <gaconju@gmail.com>
Cryoris
Cryoris reviewed Apr 30, 2025
View reviewed changes
@Cryoris
Copy link
Contributor

Cryoris commented Apr 30, 2025

Document members of QkCircuitInstruction (they're inline docstrings, but those docstrings seem to be getting lost in the
Rust -> C Header -> Breathe -> Sphinx path

This is a bit surprising, since this is how we document members in the other structs (e.g. QkObsTerm aka CSparseTerm). Could this have to do with how we designated 1 page = 1 group = 1 struct? Maybe @frankharkins can help us out here 🙂

@mtreinish @Cryoris
Update docs/cdoc/qk-circuit.rst
5de40ed 
Co-authored-by: Julien Gacon <gaconju@gmail.com>
@frankharkins
Copy link
Member

Document members of QkCircuitInstruction (they're inline docstrings, but those docstrings seem to be getting lost in the Rust -> C Header -> Breathe -> Sphinx path

I think adding the :members: option to the RST doxygengroup directive should fix that (source).

.. doxygengroup:: QkCircuit
   :members:

Also, should the struct be under "Functions"? What do you think about adding another section called "Structs" or "Data types"?

@mtreinish mtreinish requested a review from Cryoris May 8, 2025 21:29
@mtreinish
Copy link
Member Author

@frankharkins Thanks that was what was missing I've reworked things a bit based on your suggestion and I think the structure of things is a lot better now.

@mtreinish mtreinish removed the on hold Can not fix yet label May 8, 2025
Cryoris
Cryoris reviewed May 12, 2025
View reviewed changes
Cryoris
Cryoris approved these changes May 12, 2025
View reviewed changes
Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

I checked the rendered docs and the page appears and looks great 👍🏻

@Cryoris Cryoris enabled auto-merge May 12, 2025 09:15
@Cryoris Cryoris added this pull request to the merge queue May 12, 2025
Merged via the queue into Qiskit:main with commit 4c86574 May 12, 2025
24 checks passed
rahaman-quantum pushed a commit to rahaman-quantum/qiskit that referenced this pull request Jun 20, 2025
@mtreinish @Cryoris @rahaman-quantum
Improve QkCircuit C API docs (Qiskit#14259)
e08b199 
* Improve QkCircuit C API docs

This commit adds some more details to the C API docs for the new
QkCircuit struct which was added in Qiskit#14006.

* Apply suggestions from code review

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update docs/cdoc/qk-circuit.rst

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Add members option to doxygen group

* Split out data structs in docs

* Expand on current limitations of API in docs

* Other docstring fixes

* C docs don't have cross refs

---------

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
Reviewers

@Cryoris Cryoris Cryoris approved these changes

Assignees

@mtreinish mtreinish

Labels
Changelog: None Do not include in changelog documentation Something is not clear or an error documentation priority: high
Projects
None yet
Milestone
2.1.0
Development

Successfully merging this pull request may close these issues.
5 participants
@mtreinish @qiskit-bot @coveralls @Cryoris @frankharkins