-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Remove links to qiskit.org in API docs #11294
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
Conversation
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:
|
There was a problem hiding this 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:
- The sphinx cross references are missing (see the comment where I try to explain what they are and how to do them)
- 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" |
There was a problem hiding this comment.
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):
"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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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`_" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"error see: `translation_errors`_" | |
```suggestion | |
"error see: " | |
"https://docs.quantum-computing.ibm.com/api/qiskit/qiskit.transpiler.passes." | |
"BasisTranslator#translation-errors" |
There was a problem hiding this comment.
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!
Pull Request Test Coverage Report for Build 6980762916
💛 - Coveralls |
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
8c72bc4
to
4b830e8
Compare
There was a problem hiding this 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.
* 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)
Thanks so much @gluonhiggs! And thanks @ElePT for helping with the review :) |
* 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>
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/natureIn
qiskit/utils/mitigation/_filters.py
, https://qiskit.org/documentation/tutorials/noise/3_measurement_error_mitigation.html is deletedIn
qiskit/qpy/init.py
, https://qiskit.org/documentation/tutorials/circuits_advanced/05_pulse_gates.html is replacedwith https://docs.quantum-computing.ibm.com/build/pulse
In
qiskit/circuit/quantumcircuit.py
, https://qiskit.org/documentation/tutorials/circuits/3_summary_of_quantum_operations.htmlis 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 isreplaced 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 withhttps://github.com/Qiskit/qiskit/blob/main/DEPRECATION.md
In
qiskit/transpiler/passes/basis/basis_translator.py
, https://qiskit.org/documentation/stubs/qiskit.transpiler.passes.BasisTranslatoris 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.htmlis replaced with https://docs.quantum-computing.ibm.com/build/pulse