-
Notifications
You must be signed in to change notification settings - Fork 26
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
Support any two qubit gates #235
base: main
Are you sure you want to change the base?
Support any two qubit gates #235
Conversation
Codecov Report
@@ Coverage Diff @@
## main #235 +/- ##
=======================================
+ Coverage 91.9% 92.2% +0.3%
=======================================
Files 44 44
Lines 3744 3825 +81
Branches 621 628 +7
=======================================
+ Hits 3444 3530 +86
+ Misses 300 295 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Just a few comments at the end of the day: Many thanks for this addition. I think this is a great step into the right direction. In addition, I was thinking that it shouldn't be that hard to get arbitrary two-qubit gates to work (i.e., not only controlled gates, but maybe also something like an iSWAP gate or Google's fsim gate). Essentially, one has to look for places where a control and a target are expected and add some extra case handling if the considered operation has two targets and no controls. I was also wondering whether it would make sense to switch to a dictionary or separate counts for the individual two-qubit gates. Some devices, e.g., those offered by Rigetti offer multiple two-qubit gates ( Last but not least, it would be great to add some tests on the C++ side, that do try to map circuits with two-qubit gates that are not CNOTs and assert that the produced results make sense. These are all just rather general thoughts. Let me know if they make sense. |
I haven't looked that deep into qmap yet, so it's hard for me to comment on this. I would guess that the mapping algorithm doesn't care much about the semantics of the gates, so whether there are two targets or one target and one control shouldn't matter too much.
I'm not sure I understand. Are you talking about the CircuitInfo struct inside MappingResults? I would assume that the number of gates before and after the mapping are generally very similar, with the mapped circuit simply having more Hadamard or Swap gates that are used to perform the mapping. The count of a specific gate like cp doesn't seem that relevant as it should be the same before and after the mapping, right? |
Yeah, the mapping algorithm itself does not really care; it's all about connectivity. But the current code base does 🙃 |
As for the mapping itself, I tend to agree. Since it is all about the connectivity of the circuit, the existing gates will (more or less) stay the same. Probably, it is just that the result would then more closely follow what Qiskit is doing, i.e., providing a dictionary of gate counts. |
I implemented the basic case distinction we talked about to flip control and target.
|
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 a lot. Just had a quick look over this and added my comments below.
There definitely is some adaptation in the actual methodology of the mappers missing, e.g.,
https://github.com/cda-tum/qmap/blob/7e8be02b726680f22dd6e9a6d94295aedd0e7bdb/src/exact/ExactMapper.cpp#L725-L747
Needs to be adapted to assign the corresponding cost to a certain two-qubit gate that is reversed. Otherwise, the costs would be off. Gate context should be available there.
Regarding the points you brought up in the comment above:
I believe these constants should be replaced with static functions that take an OpType as input and return the corresponding cost. These functions could also be used in the functions you modified to increase the gateIdx
.
These functions could have a default OpType
of None
, which returns the existing, fixed constant value.
That would allow returning a semi-proper value even without gate context.
It's not ideal that the distanceTable
is computed without gate context, but I could live with that for now. Probably, one would need a distance table for all native gates to get this working properly.
One important thing: could you please add tests to both, the heuristic and the exact, mapper that use various two-qubit gates and assert that they are handled properly. I would be hesitant to merge this without ensuring that it really works and doesn't cause more harm than good. I guess the important cases are various controlled gates, symbolic gates, and maybe CompoundOperation
s (which are used as collections of gates in the QFR).
Thanks for the detailed feedback. |
Good question. I think it could be a collection of really small test circuits and constructed examples, where one definitely know what's best (even for the heuristic mapper). A couple of things I could think of:
The list above is probably incomplete, but should give you a rough idea of where to go. |
Sorry for the long delay. I had some troubles with the tests. After making my changes, the existing python tests no longer passed. def example():
qc1 = QuantumCircuit(3)
qc1.h(0)
qc1.cx(0, 1)
qc1.cx(1, 2)
qc1.measure_all()
qc2 = QuantumCircuit(5, 3)
qc2.h(1)
qc2.cx(1, 0)
qc2.swap(0, 2)
qc2.cx(2, 4)
qc2.barrier()
qc2.measure(1, 0)
qc2.measure(2, 1)
qc2.measure(4, 2)
result23 = verify(qc2, qc2)
qc3 = QuantumCircuit(5, 3)
qc3.h(1)
qc3.h(2)
qc3.cx(1, 0)
qc3.h(0)
qc3.cx(2, 0)
qc3.h(0)
qc3.h(2)
qc3.measure(1, 0)
qc3.measure(0, 1)
qc3.measure(2, 2)
result13 = verify(qc1, qc3)
print("1 and 3 works:", result13)
result12 = verify(qc1, qc2)
print("1 and 2 works:", result12) I constructed this example based on the tests in qmap. Here qc1 is the original circuit, qc2 is the mapped circuit when using the swap direction reverse for the distance table and qc3 is the mapped circuit when using the hadamard direction reverse for the distance table.
Is there something wrong with qc2? In particular the measure part, because replacing them with measure_all gets rid of any problems. I also tested without the swap, because it's not present in the other circuits, but even without it the same problems appear. |
No worries. That seems like a small bug in QCEC to me. Probably something related to how the Output Permutation is determined there. I'll look into that on Monday at the latest and get back to you (also on the other things in your message). |
@JoachimMarin had some time today to check the above circuits. Can confirm that there is a small bug in the ZX equivalence checker in QCEC. See cda-tum/mqt-qcec#251 for the tracking issue. A note on the circuits though: As they are, Another small remark: it would be great if you could resolve the conflicts with the |
I'm aware of that. I haven't had any issues in terms of correctness so far with qcec. I only constructed this example to show the seg fault and exception.
I merged the changes of main into this branch. I'm having problems with ruff at the moment. It told me to put the Instructions import in a TYPE_CHECKING guard because it's technically not required at runtime, but it's necessary for the test collection. |
Signed-off-by: Lukas Burgholzer <[email protected]>
Signed-off-by: Lukas Burgholzer <[email protected]>
Signed-off-by: Lukas Burgholzer <[email protected]>
I just pushed some changes refactoring the tests. Let's see how it goes in the CI. |
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.
Based on the tests running now, I took a quick look at the current state of the PR and added my comments. I think we are getting there.
Note that it would be nice to add tests on the C++ side as well to please the codecov service. Moving the existing Python tests to C++ would also increase the runtime of the tests as C++ itself is way faster than Python and no translation between Qiskit and C++ is necessary. (Verification of the results with QCEC has to be skipped on the C++ side)
If you want to avoid duplication, you could add the assertions on the gates and circuits to the C++ tests and leave the circuit verification with QCEC here in the Python tests (which would allow to unify the Python tests again into a single parameterised test).
I only considered the exact mapper so far. Swap reversals are no longer possible, instead they are realized by swap permutations and disabling reversals in the coupling constraints. The swap limit is currently problematic, because in my test cases it defaulted to zero with SwapReduction::CouplingLimit, so I disabled the swap limit for them. However, if the swap limit is enabled and too low, no solution can be found. Originally I used mostly python tests, because they made it easier to analyze the mapped circuit. To still analyze the mapped circuit, I added a function to obtain it: |
Signed-off-by: Lukas Burgholzer <[email protected]>
Signed-off-by: Lukas Burgholzer <[email protected]>
Signed-off-by: Lukas Burgholzer <[email protected]>
Signed-off-by: Lukas Burgholzer <[email protected]>
Signed-off-by: Lukas Burgholzer <[email protected]>
Signed-off-by: Lukas Burgholzer <[email protected]>
Signed-off-by: Lukas Burgholzer <[email protected]>
Signed-off-by: Lukas Burgholzer <[email protected]>
Just pushed some changes that clean up the code, add some more checks, and (most importantly) fixes the coupling limit computation. I like the addition of the getter for the mapped circuit. Unfortunately, there is no gate counting method yet that's similar to Qiskit. |
I updated the heuristic mapper, specifically the distance table to support the different costs for direction reversal. Inside updateHeuristicCost I used the cost of COST_UNIDIRECTIONAL_SWAP * 2 to guide the heuristic algorithm to make the right choice, but I'm not sure if this is correct. In general I think there is the danger for both mappers to want to use direction reverse when it's not possible. The exact mapper if the swap limit is too low and the heuristic mapper if it makes a non optimal decision. |
Thanks again. I think this needs some further changes to work properly. I just talked to @EliasLF (who is currently working on #218). That PR touches some important parts of the heuristic mapper and fixes some potential bugs. Furthermore, some of the changes there should make the changes here a little easier. Elias agreed to pull out relevant changes from the other PR into separate PRs that can be integrated quickly and act as a better basis for the PR here. I'll postpone a detailed review of the changes here until these updates are in.
The exact mapper should never run into this risk. The SWAP limit in the default mode is always chosen in a way that a satisfying mapping is possible. If the swap limit is custom set and set too low, the mapper will just return |
## Description This PR updates the internal QFR library and brings along support for many new (two-qubit) gates. Until #235 is in, the mapper in QMAP can't really take advantage of that, but it's good to be prepared for that. Some breaking changes in the QFR required changes here to make the project compile again. The Clifford Tableau simulation algorithm now also supports `iSWAP`, `DCX`, and `ECR` gates. Finally, the internal QASM parser has been improved by quite a bit; adding support for all new gates and direct parsing for natively supported gates without requiring definitions. The version specifier (`OPENQASM 2.0`) is now optional and the parser supports all standard quantum gates and controlled versions without requiring the `qelib1.inc` include. ## Checklist: <!--- This checklist serves as a reminder of a couple of things that ensure your pull request will be merged swiftly. --> - [x] The pull request only contains commits that are related to it. - [x] I have added appropriate tests and documentation. - [ ] I have made sure that all CI jobs on GitHub pass. - [x] The pull request introduces no new warnings and follows the project's style guidelines.
Some important changes have made it into main over the last couple of days. Notably
Some of these changes overlap with the changes here, causing some conflicts. As for the exact mapper, not too much has changed, so the conflicts should be fairly easy to resolve. As for the heuristic mapper, some refactoring has been done that should actually make the necessary changes in this PR a little easier as the computation of the cost for the heuristic is much more localized now. This might require a little more effort to resolve the conflicts.
All of these are two-target gates, i.e., they don't have a dedicated control and target. Most of these still have a preferred direction. They should be considered in the direction reversal as well. You can use the verification tab in https://www.cda.cit.tum.de/app/ddvis/ to check the proper direction reversal conditions. One thing that I was discussing with @EliasLF, that might turn out really well as a follow up PR to this one here, is that, at the moment, there is a global fixed cost for bi-directional and uni-directional SWAPs. Now this PR introduces (global) per gate direction reversal handling. TLDR: @JoachimMarin Could you please resolve the conflicts based on the new changes and try to adapt the existing solution in this PR? Then, we'll see where we are at. |
I have to finish my master's thesis in 3 weeks, so I don't have much time for programming at the moment. I don't know when I'll find the time for the requested changes. |
Hey @JoachimMarin 👋🏻 |
It was more work than I initially thought, so I couldn't finish it in time. I don't have the time for it, but I hope the discussion here and my work can be useful when work on two-qubit gates continues. |
Description
These changes aim to better support arbitrary two qubit gates in the mapping process.
This is done by generalizing from cnot gates to any controlled gates:
In bindings.cpp, I also renamed cnots to two_qubit_gates. I guess this would be a breaking change with python projects using qmap. I think the best course of action would be to keep both and mark cnots as deprecated.
Until qfr is updated some gates like controlled u gates will produce incorrect OpenQASM code resulting in Qiskit failing to read the mapped circuit and throwing exceptions in Python.
I'm not sure, if any further changes are required to support any two qubit gates, but I hope this serves as a good starting point.
Checklist: