Change collect_1q_runs return for performance#5685
Merged
Conversation
Currently the return type from collect_1q_runs is being converted from the 'list(list(DAGNode))' which is returned by retworkx.collect_runs() to be 'set(tuple(DAGNode))' so that it is consistent with the return type from collect_runs(). However, this ends up adding significant overhead to the collect_1q_runs method and ends up being the bottleneck. We spend more time running the set expression to convert from the list than we do in rx.collect_runs() (mainly the filter function). Since collect_1q_runs hasn't been included in a release yet we can change the return type without breaking users and avoid this extra overhead. This commit does that and removes the set comprehension and just returns the nested list directly from retworkx.collect_runs().
Member
Author
|
Looking at the profile of
without this PR. To:
With this PR applied. |
kdk
approved these changes
Jan 22, 2021
Member
kdk
left a comment
kdk
left a comment
There was a problem hiding this comment.
This looks good. Would it make sense to update the return type on the stable branch?
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Jan 22, 2021
The collect_1q_runs method was rewritten in python as part of the backport of a larger bugfix in the Optimize1qGatesDecomposition transpiler pass in Qiskit#5655. However, it is not part of the public api in 0.16.x and was only backported to fix the bug in Optimize1qGatesDecomposition. Since the version included on the master branch will be subtlely different in return type (after Qiskit#5685) Since we don't want to advertise the method until it's included in the public api this commit prepends '_' making the method name _collect_1q_runs to indicate it is internal only as part of the 0.16.2 release.
Member
Author
We can but as part of: #5686 I marked the backported python version of the function as private. So we won't be breaking any potential backwards compatibility contract if someone starts using it in 0.16.2. |
mtreinish
added a commit
that referenced
this pull request
Jan 26, 2021
* Prepare 0.16.2 release This commit prepares for the 0.16.2 release by bumping the version strings to reflect the new version. This is just a bugfix release that several fixes for bugs introduced in 0.16.1. * Mark DAGCircuit collect_1q_runs private The collect_1q_runs method was rewritten in python as part of the backport of a larger bugfix in the Optimize1qGatesDecomposition transpiler pass in #5655. However, it is not part of the public api in 0.16.x and was only backported to fix the bug in Optimize1qGatesDecomposition. Since the version included on the master branch will be subtlely different in return type (after #5685) Since we don't want to advertise the method until it's included in the public api this commit prepends '_' making the method name _collect_1q_runs to indicate it is internal only as part of the 0.16.2 release. * Cleanup release notes
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
2 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
Currently the return type from collect_1q_runs is being converted from
the 'list(list(DAGNode))' which is returned by retworkx.collect_runs()
to be 'set(tuple(DAGNode))' so that it is consistent with the return
type from collect_runs(). However, this ends up adding significant
overhead to the collect_1q_runs method and ends up being the bottleneck.
We spend more time running the set expression to convert from the list
than we do in rx.collect_runs() (mainly the filter function). Since
collect_1q_runs hasn't been included in a release yet we can change the
return type without breaking users and avoid this extra overhead. This
commit does that and removes the set comprehension and just returns the
nested list directly from retworkx.collect_runs().
Details and comments