Fix QuantumCircuit.compose on unusual types and registers
#9206
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
`QuantumCircuit.compose` previously had its own handling for converting bit specifiers into bit instances, meaning its functionality was often surprisingly less permissive than `append`, or just incorrect. This swaps the method to use the standard conversion utilities. The logic for handling mapping conditional registers was previously the old logic in `DAGCircuit`, which was called without respect to re-ordering of the clbits during the composition. The existing test case surrounding this that is modified by this commit made an incorrect assertion; in general, the condition was not the same after the composition. In this particular test case it was ok because both bits were being tested for the same value, but had the condition value been 0b01 or 0b10 it would have been incorrect. This commit updates the register-mapping logic to respect the reordering of clbits, and to create a new aliasing register to ensure the condition is represented exactly, if this is required. This fixes a category of bug where too-small registers could incorrectly be mapped to larger registers. This was not a valid transformation, because it created assertions about bits that were previously not involved in the condition's comparison.
jakelishman
added
the
Changelog: Bugfix
|
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:
|
This test was actually attempting to catch invalid state of `QuantumCircuit.compose`, but this series of commits changes the underlying function so that this test no longer needs to raise; there is valid alternative behaviour.
Pull Request Test Coverage Report for Build 3893910206
💛 - Coveralls |
Cryoris
reviewed
Jan 10, 2023
Co-authored-by: Julien Gacon <gaconju@gmail.com>
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
QuantumCircuit.composepreviously had its own handling for converting bit specifiers into bit instances, meaning its functionality was often surprisingly less permissive thanappend, or just incorrect. This swaps the method to use the standard conversion utilities.The logic for handling mapping conditional registers was previously the old logic in
DAGCircuit, which was called without respect to re-ordering of the clbits during the composition. The existing test case surrounding this that is modified by this commit made an incorrect assertion; in general, the condition was not the same after the composition. In this particular test case it was ok because both bits were being tested for the same value, but had the condition value been 0b01 or 0b10 it would have been incorrect.This commit updates the register-mapping logic to respect the reordering of clbits, and to create a new aliasing register to ensure the condition is represented exactly, if this is required. This fixes a category of bug where too-small registers could incorrectly be mapped to larger registers. This was not a valid transformation, because it created assertions about bits that were previously not involved in the condition's comparison.
Details and comments
Fix #6583
Fix #6584
Fix #8691
Note that with this change in place, the circuit visualisation tools may well not display the circuits correctly; they don't handle aliasing registers well at the moment. Similarly, I don't know precisely how well these objects will transport through the legacy Qobj paths; the circuits being described here may well not be representable in the Qobj model. If that is the case, though, it'll be up to
assembleto raise a suitable error; the representation here is what is correct forQuantumCircuit.