-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
List support for coupling map in pass manager #11063
Conversation
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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! Please could you also add a "feature" release note (reno new --edit preset-passmanager-coupling-map
) with a line saying that generate_preset_pass_manager
now accepts anything that can be coerced to CouplingMap
?
if ( | ||
coupling_map | ||
and isinstance(coupling_map, list) | ||
and all(isinstance(sublist, list) for sublist in coupling_map) | ||
): | ||
coupling_map = CouplingMap(coupling_map) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can probably do
if ( | |
coupling_map | |
and isinstance(coupling_map, list) | |
and all(isinstance(sublist, list) for sublist in coupling_map) | |
): | |
coupling_map = CouplingMap(coupling_map) | |
if coupling is not None and not isinstance(coupling_map, CouplingMap): | |
coupling_map = CouplingMap(coupling_map) |
because the CouplingMap
constructor will already have to validate its input, which would just be duplicated and liable to get out-of-sync with this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, thanks!
|
||
# Convert the transpiled circuits to DAGs to compare the states | ||
dag_list = circuit_to_dag(transpiled_circuit_list) | ||
dag_object = circuit_to_dag(transpiled_circuit_object) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but we don't really need to bother with the conversion; we can assertEqual(transpiled_circuit_list, transpiled_circuit_object)
and it'll work just as well (and give a better error message on failure).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good, I made the change. I thought it would be good practice since the pass manager doesn't consistently produce the same result. However, this shouldn't be a problem for the test circuit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, the not-the-same output is most likely because we've not set seed_transpiler
in the generate_preset_pass_manager
- good spot on that. If you set that to the same value, they should always come out the same. The QuantumCircuit
equality check actually just delegates to the DAGCircuit
one internally, so they're the same, but we have a special hook in QiskitTestCase
to make it so that assertEqual
draws the circuits in the error message if they both come in as QuantumCircuit
.
I think you've ended up with two release notes files here - could you clear out the extra one (-b4f2f0ed...
)? Other than that, this is ready to go thanks.
Thanks for the guidance and clarification! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick changes! I pushed a little fix up to the release note to add Sphinx cross-references to various objects, and to highlight that the allowed formats are more legacy-related.
Summary
Fixes #10920
Details and comments
The preset pass manager previously only allowed the user to provide a coupling map as an object, not as a list. Since the creation of a coupling map is typically a straightforward process, it makes sense to allow users to directly input the list into the preset pass manager generator. This is what this PR is about.
Example :
pm = generate_preset_pass_manager(basis_gates = ['cz', 'sx', 'rz'], coupling_map =[[0,1], [1,2]], optimization_level=3)