Update docstrings for renamed scheduling passes#7884
Merged
Conversation
In Qiskit#7835 we renamed the passes used in the new scheduler pass workflow to differentiate them from the existing legacy scheduling pass workflow (which we restored pending a future deprecation in that PR). However, while we updated the docstring for the legacy path to update them to point to the new passes, we neglected to update the docstrings for the renamed classes to reflect the new names. The examples in the docstrings still worked because of the old passes, but it wasn't showing an example of how to use the new workflow anymore. This commit corrects this oversight so that they actually explain how to use them.
mtreinish
added
stable backport potential
jakelishman
reviewed
Apr 4, 2022
Member
jakelishman
left a comment
jakelishman
left a comment
There was a problem hiding this comment.
It'll be good to get the documentation back in line with the actual behaviour.
Comment on lines
-23
to
+24
| See :class:`~qiskit.transpiler.passes.scheduling.base_scheduler.BaseScheduler` for the | ||
| detailed behavior of the control flow operation, i.e. ``c_if``. | ||
| See :class:`~qiskit.transpiler.passes.scheduling.scheduling.base_scheduler.BaseScheduler` for | ||
| the detailed behavior of the control flow operation, i.e. ``c_if``. |
Member
There was a problem hiding this comment.
I don't think this (or any similar) cross-ref will resolve; BaseScheduler isn't included in any autosummary directive.
Member
Author
There was a problem hiding this comment.
It's not, I noticed that too after I pushed it. This was existing before I just mechanically updated the paths for things. I'm not sure how we want to handle this, since to include it in the doc tree we'd probably need to re-export the class to qiskit/transpiler/passes/__init__.py but I assume @nkanazawa1989 didn't do that intentionally.
@nkanazawa1989 do you have any preference on how you want to include the BaseScheduler class in the docs?
Contributor
There was a problem hiding this comment.
Umm... I wrote detailed document about the policy of classical IO handling there, and I don't want to copy the document for every child scheduler pass. Because BaseScheduler is an abstract class, I think we don't need to expose this to module level import, but I want to provide this docs with users. Perhaps we can move document to https://qiskit.org/documentation/apidoc/transpiler.html and replace the link?
Member
Author
There was a problem hiding this comment.
I added a scheduling section to the top level transpiler docs and included these details there in: 3ae60d2
Pull Request Test Coverage Report for Build 2186571356
💛 - Coveralls |
Contributor
nkanazawa1989
left a comment
nkanazawa1989
left a comment
There was a problem hiding this comment.
Almost looking good to me. Just a nitpick. Thanks for additional docs cleanup.
nkanazawa1989
previously approved these changes
Apr 12, 2022
kevinhartman
previously approved these changes
Apr 18, 2022
mtreinish
dismissed stale reviews from kevinhartman and nkanazawa1989
via
402bdd7
Co-authored-by: Kevin Hartman <kevin@hart.mn>
kevinhartman
approved these changes
Apr 18, 2022
mergify Bot
added a commit
that referenced
this pull request
Apr 19, 2022
* Update docstrings for renamed scheduling passes In #7835 we renamed the passes used in the new scheduler pass workflow to differentiate them from the existing legacy scheduling pass workflow (which we restored pending a future deprecation in that PR). However, while we updated the docstring for the legacy path to update them to point to the new passes, we neglected to update the docstrings for the renamed classes to reflect the new names. The examples in the docstrings still worked because of the old passes, but it wasn't showing an example of how to use the new workflow anymore. This commit corrects this oversight so that they actually explain how to use them. * Add scheduling section to top level transpiler doc * Update reference * Fix lint * Fix typos Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * Apply suggestions from code review Co-authored-by: Kevin Hartman <kevin@hart.mn> Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> Co-authored-by: Kevin Hartman <kevin@hart.mn> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit a4edc11)
mergify Bot
added a commit
that referenced
this pull request
Apr 19, 2022
* Update docstrings for renamed scheduling passes In #7835 we renamed the passes used in the new scheduler pass workflow to differentiate them from the existing legacy scheduling pass workflow (which we restored pending a future deprecation in that PR). However, while we updated the docstring for the legacy path to update them to point to the new passes, we neglected to update the docstrings for the renamed classes to reflect the new names. The examples in the docstrings still worked because of the old passes, but it wasn't showing an example of how to use the new workflow anymore. This commit corrects this oversight so that they actually explain how to use them. * Add scheduling section to top level transpiler doc * Update reference * Fix lint * Fix typos Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * Apply suggestions from code review Co-authored-by: Kevin Hartman <kevin@hart.mn> Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> Co-authored-by: Kevin Hartman <kevin@hart.mn> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit a4edc11) Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
In #7835 we renamed the passes used in the new scheduler pass workflow
to differentiate them from the existing legacy scheduling pass workflow
(which we restored pending a future deprecation in that PR). However,
while we updated the docstring for the legacy path to update them to
point to the new passes, we neglected to update the docstrings for the
renamed classes to reflect the new names. The examples in the docstrings
still worked because of the old passes, but it wasn't showing an example
of how to use the new workflow anymore. This commit corrects this
oversight so that they actually explain how to use them.
Details and comments