Add Expr support to DAGCircuit.compose
#10377
Merged
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
jakelishman
added
the
Changelog: New Feature
|
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 5602299220
💛 - Coveralls |
jakelishman
force-pushed
the
expr/dag-compose
branch
from
e0493bc
to
ac942ee
Compare
This was referenced Jul 4, 2023
The remapping facilities are (except for some esoteric behaviour for mapping registers to others in a different order) the same as for `QuantumCircuit`, so it makes sense to unify the handling. As part of switching the old `DAGCircuit` classical-resource-mapping methods over to the new general-purpose forms, this does most of the necessary work to upgrade `substitute_node_with_dag` as well.
jakelishman
force-pushed
the
expr/dag-compose
branch
from
ac942ee
to
aa50bec
Compare
|
Now rebased over |
Comment on lines
34
to
37
| If an ``add_register`` callable is given to the initialiser, the mapper will use it to attempt | ||
| to add new aliasing registers to the outer circuit object, if there is not already a suitable | ||
| register for the mapping available in the circuit. If this parameter is not given, the mapping | ||
| function will raise an exception of type ``exc_type`` instead. |
There was a problem hiding this comment.
Can we instead just leave it up to the caller to throw their own exception from the add_register callable they pass in?
It looks like we only use this once in DAGCircuit, and IMO the extra parameter (and thusly required defaulting) and docstring aren't worth it given that this is a private interface.
There was a problem hiding this comment.
Yeah, that sounds reasonable. Doing that marginally compromises the available error message (since you don't get the original register), but it's probably not so important. I've done a version of this in 591cbaa.
Instead, we just delegate this down to a passed-in `add_register`, if desired. It marginally compromises the error messages that might be emitted, but those are paths that we're hope to reduce use of anyway.
kevinhartman
approved these changes
Jul 19, 2023
to24toro
pushed a commit
to to24toro/qiskit-terra
that referenced
this pull request
Aug 3, 2023
* Add `Expr` support to `DAGCircuit.compose` The remapping facilities are (except for some esoteric behaviour for mapping registers to others in a different order) the same as for `QuantumCircuit`, so it makes sense to unify the handling. As part of switching the old `DAGCircuit` classical-resource-mapping methods over to the new general-purpose forms, this does most of the necessary work to upgrade `substitute_node_with_dag` as well. * Remove `exc_type` argument from `VariableMapper` Instead, we just delegate this down to a passed-in `add_register`, if desired. It marginally compromises the error messages that might be emitted, but those are paths that we're hope to reduce use of anyway.
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
The remapping facilities are (except for some esoteric behaviour for mapping registers to others in a different order) the same as for
QuantumCircuit, so it makes sense to unify the handling. As part of switching the oldDAGCircuitclassical-resource-mapping methods over to the new general-purpose forms, this does most of the necessary work to upgradesubstitute_node_with_dagas well.Details and comments
Resolve #10230. Depends on #10375 (since I elected to generalise some of the new handling from that PR). Changelog in #10331.