Recreate full dag instead of inplace substitution in BasisTranslator #12195
Conversation
This file contains 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
This commit tweaks the internal logic of the basis translator transpiler pass to do a full dag recreation instead of inplace modification. If only a few operations were to be substituted it would probably be more efficient to do an inplace modification, but in general the basis translator ends up replacing far more operations than not. In such cases just iterating over the dag and rebuilding it is more efficient because the overhead of `apply_operation_back()` is minimal compared to `substitute_node_with_dag()` (although it's higher than `subtitute_node(.., inplace=True)`).
|
One or more of the the following people are requested to review this:
|
|
I ran some asv benchmarks for this PR here: The |
ElePT
reviewed
Apr 23, 2024
There was a problem hiding this comment.
The code looks straightforward, and it looks good to me. About the large_qasm benchmark, I think that it might be worth benchmarking for the 2% crossover point and potentially adding the switch. It seems relatively low effort and avoids a regression in a use case that doesn't seem very useful for practical applications, but looks likely to come up in benchmarking efforts (just thinking of people thinking of edge cases, or lazily constructing circuits). This is just my opinion, of course. Another small comment is that the circuit drawer unit test is acting out.
Weird the output circuit from the transpiler of an if statement's body is shifting from rz(pi) to rz(-pi) with a global phase of pi. So both outputs are equivalent but the visualization changes and because we don't display global phase inside control flow bodies it looks a bit weird now. I'm not sure what is causing this though because I didn't change the basis translators logic, just how we construct the output I wouldn't have expected any changes. |
ElePT
reviewed
Apr 30, 2024
Comment on lines
-267
to
-269
| if dag_updated: | ||
| flow_circ_block = dag_to_circuit(dag_block) |
There was a problem hiding this comment.
I think that this might be the origin of the test failure. In the original code, flow_circ_block = dag_to_circuit(dag_block) but in the updated one flow_circ_block = dag_to_circuit(updated_dag). Replacing updated_dag with dag_block fixes the test locally for me, however, I think that the correct line should be flow_circ_block = dag_to_circuit(updated_dag) (else, why would you update it and not use it?). Do you think this could have been an oversight in the original code and the test just got dragged with it?
There was a problem hiding this comment.
Hmm, in the original version of this code dag_updated was a boolean that indicated whether the input dag to apply_translation() was transformed at all. If it was we would add the modified dag to the flow_blocks list which then would get updated. Otherwise we would use the original input block unmodified. I guess maybe we want to have some logic in place to replicate the no-substitution path that I kind of clobbered here.
|
I pushed this out to 1.2.0 since I haven't had a chance to fix the root cause of the test failure. That being said this might be superseded by #12246 |
…riginal 'flow_blocks' logic and fix drawer test.
|
I tried fixing the latest conflicts and proposing a solution for the failed test. To make the code work after merging the changes from #12705 I ended up applying a few hacky-looking fixes (such as re-writing |
Pull Request Test Coverage Report for Build 10148007745Details
💛 - Coveralls |
|
I ran the benchmarks after @ElePT's fix and rebase: |
ElePT
approved these changes
Jul 30, 2024
There was a problem hiding this comment.
Thanks @mtreinish for running the benchmarks! It's not a huge improvement but seems to be useful for the utility scale benchmarks. I'd go ahead with the PR, what do you think?
ElePT
added
the
stable backport potential
mergify bot
pushed a commit
that referenced
this pull request
Jul 30, 2024
…12195) * Recreate full dag instead of inplace substitution in BasisTranslator This commit tweaks the internal logic of the basis translator transpiler pass to do a full dag recreation instead of inplace modification. If only a few operations were to be substituted it would probably be more efficient to do an inplace modification, but in general the basis translator ends up replacing far more operations than not. In such cases just iterating over the dag and rebuilding it is more efficient because the overhead of `apply_operation_back()` is minimal compared to `substitute_node_with_dag()` (although it's higher than `subtitute_node(.., inplace=True)`). * Return boolean together with dag in 'apply_translation' to maintain original 'flow_blocks' logic and fix drawer test. * Remove print --------- Co-authored-by: Elena Peña Tapia <[email protected]> (cherry picked from commit 37b334f)
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 30, 2024
…12195) (#12855) * Recreate full dag instead of inplace substitution in BasisTranslator This commit tweaks the internal logic of the basis translator transpiler pass to do a full dag recreation instead of inplace modification. If only a few operations were to be substituted it would probably be more efficient to do an inplace modification, but in general the basis translator ends up replacing far more operations than not. In such cases just iterating over the dag and rebuilding it is more efficient because the overhead of `apply_operation_back()` is minimal compared to `substitute_node_with_dag()` (although it's higher than `subtitute_node(.., inplace=True)`). * Return boolean together with dag in 'apply_translation' to maintain original 'flow_blocks' logic and fix drawer test. * Remove print --------- Co-authored-by: Elena Peña Tapia <[email protected]> (cherry picked from commit 37b334f) Co-authored-by: Matthew Treinish <[email protected]>
Procatv
pushed a commit
to Procatv/qiskit-terra-catherines
that referenced
this pull request
Aug 1, 2024
…iskit#12195) * Recreate full dag instead of inplace substitution in BasisTranslator This commit tweaks the internal logic of the basis translator transpiler pass to do a full dag recreation instead of inplace modification. If only a few operations were to be substituted it would probably be more efficient to do an inplace modification, but in general the basis translator ends up replacing far more operations than not. In such cases just iterating over the dag and rebuilding it is more efficient because the overhead of `apply_operation_back()` is minimal compared to `substitute_node_with_dag()` (although it's higher than `subtitute_node(.., inplace=True)`). * Return boolean together with dag in 'apply_translation' to maintain original 'flow_blocks' logic and fix drawer test. * Remove print --------- Co-authored-by: Elena Peña Tapia <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
This commit tweaks the internal logic of the basis translator transpiler pass to do a full dag recreation instead of inplace modification. If only a few operations were to be substituted it would probably be more efficient to do an inplace modification, but in general the basis translator ends up replacing far more operations than not. In such cases just iterating over the dag and rebuilding it is more efficient because the overhead of
apply_operation_back()is minimal compared tosubstitute_node_with_dag()(although it's higher thansubtitute_node(.., inplace=True)).Details and comments