Skip to content
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 reverse permutation for transpiled circuits in Operator.from_circuit #8802

Merged
merged 7 commits into from
Oct 5, 2022

Conversation

mtreinish
Copy link
Member

Summary

This commit fixes a bug in the Operator.from_circuit() constructor method for Layout's generated by transpile(). The transpiler would set the _layout property for an output circuit to be the mapping from the input circuit's virtual qubits to the physical qubits (also the output circuit's qubit index). This is useful for visualization and visually tracking the permutation assuming you have the original circuit. However for the Operator.from_circuit() constructor method which will reverse the permutation caused by layout in the generated matrix this is not sufficient information since we need to order of the original circuits qubits. To fix this issue this commit changes the _layout attribute of the QuantumCircuit class to store both the initial layout as before and also a mapping of qubit objects to indices from the original circuit. Using this extra information we can reliably handle the qubit permutation in the constructor.

Details and comments

Fixes #8800

This commit fixes a bug in the Operator.from_circuit() constructor
method for Layout's generated by transpile(). The transpiler would set
the _layout property for an output circuit to be the mapping from the
input circuit's virtual qubits to the physical qubits (also the output
circuit's qubit index). This is useful for visualization and visually
tracking the permutation assuming you have the original circuit. However
for the `Operator.from_circuit()` constructor method which will reverse
the permutation caused by layout in the generated matrix this is not
sufficient information since we need to order of the original circuits
qubits. To fix this issue this commit changes the `_layout` attribute of
the QuantumCircuit class to store both the initial layout as before and
also a mapping of qubit objects to indices from the original circuit.
Using this extra information we can reliably handle the qubit
permutation in the constructor.

Fixes Qiskit#8800
@mtreinish mtreinish added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Sep 28, 2022
@mtreinish mtreinish added this to the 0.22 milestone Sep 28, 2022
@qiskit-bot
Copy link
Collaborator

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:

@coveralls
Copy link

coveralls commented Sep 28, 2022

Pull Request Test Coverage Report for Build 3189095581

  • 27 of 29 (93.1%) changed or added relevant lines in 10 files are covered.
  • 31 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.02%) to 84.674%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/visualization/circuit/matplotlib.py 2 3 66.67%
qiskit/visualization/gate_map.py 2 3 66.67%
Files with Coverage Reduction New Missed Lines %
src/sabre_swap/mod.rs 2 95.05%
src/results/marginalization.rs 29 68.97%
Totals Coverage Status
Change from base Build 3188378760: -0.02%
Covered Lines: 61662
Relevant Lines: 72823

💛 - Coveralls

Didn't see this locally because cplex is not installed.
@alexanderivrii
Copy link
Contributor

alexanderivrii commented Oct 3, 2022

@mtreinish, one quick question, to see if I understand things correctly: with this PR we would be able to reliably check equivalence of the original and the transpiled circuits by checking Operator(circuit).equiv(result), even if the transpiler chose a non-trivial initial layout and introduced swaps. However, the above equivalence check would not work if ancillas were allocated. (And of course if the number of qubits is sufficiently large, something like 20, but I don't recall the exact numbers, the Operator cannot be constructed either).

@mtreinish
Copy link
Member Author

@alexanderivrii Yeah, that's the basic idea of this (except it's Operator.from_circuit(result), assuming result is the post transpiled circuit). If ancillas are added this doesn't factor that in currently but maybe we can add a ignore_idle_qubits argument in a follow up PR (for 0.23.0) to look at enabling that. As for the max size it will depend on memory but for larger number of qubits the memory requires are quite large 17 qubits requires 128GB of memory to allocate the array.

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A minor comment about potentially changing the type of the _layout attribute, but mostly this looks fine to me.

qiskit/transpiler/basepasses.py Outdated Show resolved Hide resolved
test/python/quantum_info/operators/test_operator.py Outdated Show resolved Hide resolved
@jakelishman jakelishman added the Changelog: API Change Include in the "Changed" section of the changelog label Oct 4, 2022
This commit updates the _layout QuantumCircuit attribute to store a new
dataclass, TranspileLayout, which currently stores the initial_layout
used by the transpiler and the initial qubit mapping which maps the
qubit object to it's position index in the circuit. For 0.23.0 we plan
to make the circuit layout attribute public and having a proper
container class will make it easier to do this in a way where we can
evolve the interface over time if needed (likely adding a final_layout
attribute in the near future). Right now the class isn't advertised or
re-exported as the interface is still private/internal but we can make
it a public interface at the same time we add a public getter for the
circuit layout.
jakelishman
jakelishman previously approved these changes Oct 4, 2022
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, and good to get the bug fixed!

Copy link
Contributor

@alexanderivrii alexanderivrii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delayed reply -- it took me some time to understand the intricacies of this issue and why the original layout alone is not sufficient to fully reconstruct the operator from the transpiled circuit. And, indeed, Layout stores a mapping from indices of physical qubits to things like Qubit(QuantumRegister(3, 'q2'), 2) (corresponding to virtual qubits in the original circuit), but does not store the indices of these Qubit(QuantumRegister(3, 'q2'), 2) virtual qubits in the original circuit. The current PR addresses this by defining TranspilerLayout that contains both the layout and these indices (called input_qubit_mapping). The Operator.from_circuit has been modified to factor this input_qubit_mapping into construction. Overall, this LGTM and constitutes an important step to enable checking the equivalence of the original and the transpiled circuits.

However, things are still a bit trickier than described, and more work is needed to fully check the equivalence of the original and the transpiled circuits. For one, the layout also contains things like Qubit(QuantumRegister(1, 'ancilla'), 0) (corresponding to auxiliary qubits not present in the original circuit). On the positive side, input_qubit_mapping has these as well (mapping these to indices outside of the range of the original qubits). Not all of these ancilla qubits are in general idle, so possibly the equivalence check has to be done for the [original circuit extended with the appropriate number of ancilla qubits] vs. the [transpiled circuit]. But possibly completely idle qubits can be removed from the analysis. For two, as far as I can tell, we might still be missing the final permutation, for instance RemoveSwapsBeforeMeasure can remove the trailing swaps in the circuit and this information is not reflected in TranspilerLayout.

@mtreinish
Copy link
Member Author

Right, this alone is not sufficient to fully reverse the permutation caused by the transpiler in Operator.from_circuit.This PR is about fixing the initial layout support that already existed in .from_circuit, #8597 is intended to deal with the output permutation caused by swap mapping. That change depends on this pr now, I split this out as to fix the initial layout handling became more involved than what I should integrate into that pr. As for ancilla handling we'll have to do that in a follow-up to #8597 since I think it will require a bit more work than what I have scoped in that pr.

@mergify mergify bot merged commit b7e6329 into Qiskit:main Oct 5, 2022
@mtreinish mtreinish deleted the fix-from-circuit-permutation branch October 5, 2022 13:01
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Oct 5, 2022
As part of this update the final_layout is integrated into the
TranspileLayout class which was added in Qiskit#8802 instead of a standalone
attribute on the quantum circuit.
burgholzer added a commit to cda-tum/mqt-qcec that referenced this pull request Oct 24, 2022
This PR makes QCEC compatible with the newest `qiskit-terra` release,
which changed some internals of the `QuantumCircuit` class that broke
the circuit import for compiled circuits in QCEC.

The cause for this issue was the change in
Qiskit/qiskit#8802.
The (backwards-compatible) solution has been implemented in
cda-tum/mqt-core#207.

This PR also drops the upper-cap on the `qiskit-terra` version easing
the integration with future versions of qiskit.

Signed-off-by: burgholzer <burgholzer@me.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog Changelog: Bugfix Include in the "Fixed" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Operator.from_circuit fails on transpiled circuits with user-defined registers
6 participants