-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fix input normalisation of transpile(initial_layout=...)
#11031
Conversation
…ormalize input of range to a list
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 looking at this, Henry. Could you add:
- a brief test in
test/python/compiler/test_transpile.py
that atranspile
call withinitial_layout=range(...)
returns the same result as a call withinitial_layout=list(range(...))
(for a fixed transpilation seed) - a bugfix release note about this?
qiskit/compiler/transpiler.py
Outdated
if isinstance(initial_layout, range): | ||
return list(initial_layout) | ||
if all(phys is None or isinstance(phys, Qubit) for phys in initial_layout): | ||
return Layout.from_qubit_list(initial_layout) |
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.
I think at this point we could just do initial_layout = list(initial_layout)
to handle any iterable? At worst it'll make a copy of an existing list, but the list isn't going to be large, so it's not a big deal.
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.
Sure thing! I've added the test and release note, and I agree that we can just use initial_layout = list(initial_layout)
. I made that adjustment.
transpile(initial_layout=...)
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 PR Henry! The test looks good.
I pushed a couple of minor tweaks, one to add a couple of cross-references to the release note, and one to let us handle consumable iterables (like generators). Sorry for stepping on your toes, I'm just trying to get this in the merge queue asap.
* Fixed issue10554 by modifying _parse_initial_layout in transpile to normalize input of range to a list * Added test and bugfix note * Handle consumable iterables * Fix release-note cross references --------- Co-authored-by: Jake Lishman <[email protected]> (cherry picked from commit 6148f90) # Conflicts: # test/python/transpiler/test_transpile_layout.py
…11031) (#11058) * Fix input normalisation of `transpile(initial_layout=...)` (#11031) * Fixed issue10554 by modifying _parse_initial_layout in transpile to normalize input of range to a list * Added test and bugfix note * Handle consumable iterables * Fix release-note cross references --------- Co-authored-by: Jake Lishman <[email protected]> (cherry picked from commit 6148f90) # Conflicts: # test/python/transpiler/test_transpile_layout.py * Fix backport merge conflicts The backport of this commit from main failed because the test for this bug fix was added to an existing file on the main branch but that didn't exist on the stable/0.25 branch. To fix this the test is moved to a separate standalone file. Arguably, this should be done on main and stable/0.45 too as the file the test was added to was for a different component. --------- Co-authored-by: Henry Zou <[email protected]> Co-authored-by: Matthew Treinish <[email protected]>
…ormalize input of range to a list
Summary
Fixes issue10554.
Details and comments
In transpile, when we are parsing through the initial layout, it misses the case of range inputs, which caused things like
transpile(qc, initial_layout=range(2))
to raise the errorTranspilerError: "SetLayout was intialized with the layout type: <class 'range'>"
. This PR changes the_parsee_initial_layout
intranspile
to also check for case where theinitial_layout
is an instance of range, and changes it to a list.Note, this essentially makes
transpile(qc, initial_layout=range(2))
equivalent totranspile(qc, initial_layout=[0,1])