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

added documentation for get_standard_gate_name_mapping #13230

Merged

Conversation

Shivansh20128
Copy link
Contributor

Summary

Added documentation for the function get_standard_gate_name_mapping() in the Standard Gates section here.
#Closes #13108

Details and comments

I have added the necessary details about the function with code examples in the documentation.

@Shivansh20128 Shivansh20128 requested a review from a team as a code owner September 27, 2024 11:37
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Sep 27, 2024
@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 following people are relevant to this code:

  • @Cryoris
  • @Qiskit/terra-core
  • @ajavadia

@coveralls
Copy link

coveralls commented Sep 27, 2024

Pull Request Test Coverage Report for Build 11716766133

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

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 767 unchanged lines in 29 files lost coverage.
  • Overall coverage decreased (-0.02%) to 88.78%

Files with Coverage Reduction New Missed Lines %
qiskit/compiler/transpiler.py 1 93.14%
crates/accelerate/src/unitary_synthesis.rs 1 92.2%
qiskit/transpiler/target.py 1 96.13%
crates/qasm2/src/expr.rs 1 94.02%
qiskit/circuit/library/arithmetic/adders/draper_qft_adder.py 1 96.97%
qiskit/circuit/parameter.py 1 98.36%
qiskit/synthesis/unitary/qsd.py 1 97.58%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 2 58.05%
qiskit/transpiler/passes/optimization/consolidate_blocks.py 2 96.15%
qiskit/transpiler/preset_passmanagers/generate_preset_pass_manager.py 3 97.83%
Totals Coverage Status
Change from base Build 11690680453: -0.02%
Covered Lines: 77780
Relevant Lines: 87610

💛 - Coveralls

Copy link
Member

@1ucian0 1ucian0 left a comment

Choose a reason for hiding this comment

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

Thanks! My understanding of the OP message in the issue is to extend qiskit.circuit.library.standard_gates.get_standard_gate_name_mapping docstring. I think it makes sense to mention it too at module level.

@Shivansh20128
Copy link
Contributor Author

Thanks! My understanding of the OP message in the issue is to extend qiskit.circuit.library.standard_gates.get_standard_gate_name_mapping docstring. I think it makes sense to mention it too at module level.

I did not understand what you mean by that. Do I need to add this documentation somewhere else?

Copy link
Member

@1ucian0 1ucian0 left a comment

Choose a reason for hiding this comment

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

Do I need to add this documentation somewhere else?

Yes. The documentation should go under def get_standard_gate_name_mapping in qiskit/circuit/library/standard_gates/__init__.py.

@Shivansh20128
Copy link
Contributor Author

Do I need to add this documentation somewhere else?

Yes. The documentation should go under def get_standard_gate_name_mapping in qiskit/circuit/library/standard_gates/__init__.py.

Okay. I will move it there.
Thank you

Copy link
Collaborator

@Eric-Arellano Eric-Arellano 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 working on this!

Comment on lines 130 to 132
get_standard_gate_name_mapping()
-------------------------------------
It returns a dictionary mapping the name of standard gates and instructions to an object for that name.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This whole section should be removed. Instead, you're going to use .. autofunction to auto-populate the docs from the function docstring.

I'm not sure yet where the Qiskit devs will want this function to show up on https://docs.quantum.ibm.com/api/qiskit/circuit_library. So for now, you could put a new section called Utility functions (section TBD) at the bottom of this module docstring. Use == rather than -- for the header type, and make sure the header is at least as long as the text above it.

Then, you'll put this under the header:

.. autofunction:: get_standard_gate_name_mapping

However, you'll need to make one more change. Right now, you have to import the full qiskit.circuit.library.standard_gates rather than being able to import qiskit.circuit.library. You can set up this import alias by adding to the bottom of the file from .standard_gates import get_standard_gate_name_mapping. That will allow both imports to work, and it will get the Sphinx docs working correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. I have made the required changes in the new commits.
Thank you

Comment on lines 53 to 62
For Example:

For the identity gate, the dictionary contains a mapping-

.. parsed-literal::
'id': Instruction(name='id', num_qubits=1, num_clbits=0, params=[])

Note here that the object on the right belongs to the broad class:

* :class:`qiskit.circuit.Instruction`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this example is necessary. The example you have below it is already good. It's also useful to have the following paragraph before the example so that you are first told what to expect, rather than having to infer it from the example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I have removed this example.
Thank you

Copy link
Collaborator

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

The CI failure is not from your change - it was unrelated. Pulling main into your branch (usually git pull upstream main) should fix it.

Shivansh20128 and others added 2 commits October 3, 2024 19:32
Comment on lines 130 to 133
Utility functions (section TBD)
================================

.. autofunction:: get_standard_gate_name_mapping
Copy link
Collaborator

Choose a reason for hiding this comment

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

Qiskit devs: do you have any suggestions on what to name the section and where it makes sense to live in this module page? I don't use the circuit library enough to have an opinion.

Copy link
Member

Choose a reason for hiding this comment

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

I put it in the prelude (without a section) as it is the only one (see 18693b7). If, in the future, we get more, we can create a tooling section. what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

It looks like this:
get_standard_gate_name_mapping

If a native speaker can check the English, we are good to merge this one.

Copy link
Member

Choose a reason for hiding this comment

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

If a native speaker can check the English, we are good to merge this one.

@Eric-Arellano ?

Eric-Arellano
Eric-Arellano previously approved these changes Nov 6, 2024
Copy link
Collaborator

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thanks @1ucian0 , this works for me to be in the prelude since it helps you to discover what is available

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
@1ucian0
Copy link
Member

1ucian0 commented Nov 7, 2024

Thanks both!

@1ucian0 1ucian0 added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Nov 7, 2024
@Eric-Arellano Eric-Arellano added this pull request to the merge queue Nov 7, 2024
Merged via the queue into Qiskit:main with commit 3889fe0 Nov 7, 2024
18 checks passed
mergify bot pushed a commit that referenced this pull request Nov 7, 2024
* added documentation for get_standard_gate_name_mapping

* doc fix at line 145

* Update qiskit/circuit/library/__init__.py

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>

* added docs get_standard_gate_name_mapping

* reformatting at line 65-66

* update fix formatting

* Update qiskit/circuit/library/standard_gates/__init__.py

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>

* docfix using autofunction

* content updated get_standard_gate_name_mapping

* Update qiskit/circuit/library/standard_gates/__init__.py

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>

* Update qiskit/circuit/library/__init__.py

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>

---------

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
(cherry picked from commit 3889fe0)

# Conflicts:
#	qiskit/circuit/library/__init__.py
@raynelfss raynelfss added the Changelog: None Do not include in changelog label Nov 7, 2024
@1ucian0
Copy link
Member

1ucian0 commented Nov 8, 2024

@Mergifyio backport stable/1.3

Copy link
Contributor

mergify bot commented Nov 8, 2024

backport stable/1.3

✅ Backports have been created

  • Backport to branch stable/1.3 not needed, change already in branch stable/1.3

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 Community PR PRs from contributors that are not 'members' of the Qiskit repo stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Document get_standard_gate_name_mapping
6 participants