Skip to content

Conversation

gluonhiggs
Copy link
Contributor

Summary

fixes #11289

Details and comments

Removed links to qiskit.org in API docs::
In qiskit/utils/optionals.py, https://qiskit.org/documentation/nature is replaced with https://qiskit.org/ecosystem/nature
In qiskit/utils/mitigation/_filters.py, https://qiskit.org/documentation/tutorials/noise/3_measurement_error_mitigation.html is deleted
In qiskit/qpy/init.py, https://qiskit.org/documentation/tutorials/circuits_advanced/05_pulse_gates.html is replaced
with https://docs.quantum-computing.ibm.com/build/pulse
In qiskit/circuit/quantumcircuit.py, https://qiskit.org/documentation/tutorials/circuits/3_summary_of_quantum_operations.html
is replaced with https://docs.quantum-computing.ibm.com/build/circuit-construction
In qiskit/circuit/library/phase_estimation.py, https://learn.qiskit.org/course/ch-algorithms/quantum-phase-estimation is
replaced with https://github.com/Qiskit/textbook/blob/main/notebooks/ch-algorithms/quantum-phase-estimation.ipynb
In qiskit/qasm3/init.py, https://qiskit.org/documentation/deprecation_policy.html is replaced with
https://github.com/Qiskit/qiskit/blob/main/DEPRECATION.md
In qiskit/transpiler/passes/basis/basis_translator.py, https://qiskit.org/documentation/stubs/qiskit.transpiler.passes.BasisTranslator
is replaced with Sphinx cross-reference
In qiskit/transpiler/passes/scheduling/alignments/pulse_gate_validation.py, https://qiskit.org/documentation/tutorials/circuits_advanced/05_pulse_gates.html
is replaced with https://docs.quantum-computing.ibm.com/build/pulse

@gluonhiggs gluonhiggs requested review from a team and woodsp-ibm as code owners November 22, 2023 13:51
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Nov 22, 2023
@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:

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

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Thank you for picking up this issue so quickly! Most of the changes look good to me, I only have 2 comments:

  1. The sphinx cross references are missing (see the comment where I try to explain what they are and how to do them)
  2. Docs changes are not included in the changelog, so the correct action in this case is not to add a release note. Thanks though for going through the effort of making it :)

Once these comments are addressed, the PR should be ready to go.

@@ -202,8 +202,7 @@ def run(self, dag):
"target basis is not universal or there are additional equivalence rules "
"needed in the EquivalenceLibrary being used. For more details on this "
"error see: "
"https://qiskit.org/documentation/stubs/qiskit.transpiler.passes."
"BasisTranslator.html#translation_errors"
"Sphinx cross-reference"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sphinx cross-references are links to other parts of the docs, you can check out the sphinx documentation to see different strategies to incorporate cross-references into the text. In this case, you would want to link to the "translation_errors" section of the BasisTranslator docstring. I think that there is already a tag you can use on line 86, so the cross-reference link should work if you do (check out the explicit reference section in the sphinx docs above):

Suggested change
"Sphinx cross-reference"
`translation_errors`_

@@ -220,8 +219,7 @@ def run(self, dag):
f"basis: {list(target_basis)}. This likely means the target basis is not universal "
"or there are additional equivalence rules needed in the EquivalenceLibrary being "
"used. For more details on this error see: "
"https://qiskit.org/documentation/stubs/qiskit.transpiler.passes.BasisTranslator."
"html#translation_errors"
"Sphinx cross-reference"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above :) I think you can also move the cross reference to the line above.

Copy link
Contributor Author

@gluonhiggs gluonhiggs Nov 23, 2023

Choose a reason for hiding this comment

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

Please have a look! I added a commit as you suggested.

@ElePT ElePT added documentation Something is not clear or an error documentation Changelog: None Do not include in changelog labels Nov 22, 2023
Copy link
Contributor

@ElePT ElePT 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 applying my suggestion. I took a closer look at the built documentation to make sure the links worked, and realized that the sphinx cross references we discussed were not showing up. Then I expanded the code and realized that they are in TranspileError messages, which display plain text, and are not parts of the docs per se. I am reverting my suggestion and suggesting a new link instead, as the sphinx references will not work. This was probably an oversight in the initial analysis of the issue.

"error see: "
"https://qiskit.org/documentation/stubs/qiskit.transpiler.passes."
"BasisTranslator.html#translation_errors"
"error see: `translation_errors`_"
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
"error see: `translation_errors`_"
```suggestion
"error see: "
"https://docs.quantum-computing.ibm.com/api/qiskit/qiskit.transpiler.passes."
"BasisTranslator#translation-errors"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked the link and added a new commit!

@coveralls
Copy link

coveralls commented Nov 23, 2023

Pull Request Test Coverage Report for Build 6980762916

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 10 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.01%) to 87.021%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 4 91.41%
crates/qasm2/src/parse.rs 6 97.6%
Totals Coverage Status
Change from base Build 6980540179: 0.01%
Covered Lines: 60860
Relevant Lines: 69937

💛 - Coveralls

gluonhiggs and others added 2 commits November 23, 2023 20:51
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
@ElePT ElePT force-pushed the resolve_issue_#11289 branch from 8c72bc4 to 4b830e8 Compare November 24, 2023 12:28
Copy link
Contributor

@ElePT ElePT 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 a lot, there was a merge conflict because of a PR merged a few minutes ago but I have handled it myself.

@ElePT ElePT enabled auto-merge November 24, 2023 12:32
@ElePT ElePT added this pull request to the merge queue Nov 24, 2023
Merged via the queue into Qiskit:main with commit 09ccfcf Nov 24, 2023
@Eric-Arellano Eric-Arellano added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Nov 24, 2023
@Eric-Arellano Eric-Arellano added this to the 0.45.1 milestone Nov 24, 2023
mergify bot pushed a commit that referenced this pull request Nov 24, 2023
* Remove links to qiskit.org in API docs

* remove release note and fix cross reference

* Update qiskit/transpiler/passes/basis/basis_translator.py

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

* use direct link of translation_errors

---------

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Co-authored-by: Elena Peña Tapia <epenatap@gmail.com>
(cherry picked from commit 09ccfcf)
@Eric-Arellano
Copy link
Collaborator

Thanks so much @gluonhiggs! And thanks @ElePT for helping with the review :)

github-merge-queue bot pushed a commit that referenced this pull request Nov 24, 2023
* Remove links to qiskit.org in API docs

* remove release note and fix cross reference

* Update qiskit/transpiler/passes/basis/basis_translator.py

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

* use direct link of translation_errors

---------

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Co-authored-by: Elena Peña Tapia <epenatap@gmail.com>
(cherry picked from commit 09ccfcf)

Co-authored-by: Duong H. D <79622476+gluonhiggs@users.noreply.github.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 Community PR PRs from contributors that are not 'members' of the Qiskit repo documentation Something is not clear or an error documentation 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.

Remove links to qiskit.org in API docs
5 participants