⚡ Added native construction of two-target controlled-gate DDs #534

BertiFlorea · 2024-01-16T19:14:25Z

Description

The purpose of this PR is to add the extra logic for two-target controlled-gate DDs mentioned in #334 (comment). The problem was that the current implementation of the makeTwoQubitGateDD only worked for non-controlled two-target gates. Additionally, what we also want to achieve with this PR is to get rid of all the extra methods using the previous workaround (i.e.https://github.com/cda-tum/dd_package/blob/7c9644a2a65742bb580e3e5fefeac075c9534351/include/dd/Package.hpp#L629-L810).

At the moment the updated version of makeTwoQubitGateDD is passing the CI, but there are still some TODOs left:

test for equivalence between old-fashioned workaround methods and makeTwoQubitGateDD
update the code across the package (e.g. getStandardOperationDD)
verify the functionality by testing for edge cases

Fixes #334

Checklist:

The pull request only contains commits that are related to it.
I have added appropriate tests and documentation.
I have made sure that all CI jobs on GitHub pass.
The pull request introduces no new warnings and follows the project's style guidelines.

codecov · 2024-01-16T19:17:51Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (e10ac10) 91.3% compared to head (0ba9f8b) 91.2%.

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #534     +/-   ##
=======================================
- Coverage   91.3%   91.2%   -0.2%     
=======================================
  Files        130     130             
  Lines      13857   13711    -146     
  Branches    2153    2155      +2     
=======================================
- Hits       12665   12512    -153     
- Misses      1192    1199      +7

Flag	Coverage Δ		*Carryforward flag
cpp	`90.9% <90.2%> (-0.2%)`	⬇️	Carriedforward from d7193ef
python	`99.7% <ø> (ø)`

*This pull request uses carry forward flags. Click here to find out more.

Files	Coverage Δ
include/dd/GateMatrixDefinitions.hpp	`100.0% <ø> (ø)`
include/dd/Operations.hpp	`89.1% <97.3%> (-2.6%)`	⬇️
include/dd/Package.hpp	`96.1% <84.0%> (-0.9%)`	⬇️

burgholzer

Overall the changes here are looking good and it seems like this should be working.
The biggest thing that is missing is tests of the new functionality. That's also why the code coverage check is failing. Almost none of the new lines (part of the patch) are covered by tests.

This should be quickly resolved though once the existing workaround methods are removed.
I would propose to turn the existing workarounds for constructing controlled two-target DDs into tests for the new functionality. That way, the existing code is not lost and is useful for testing.

include/dd/Package.hpp

Co-authored-by: Lukas Burgholzer <burgholzer@me.com>

…ontrolled-gate-DDs

✅ improved code coverage by adding new edge cases and updated old ones

BertiFlorea · 2024-01-21T15:38:04Z

At the moment, the following progress has been made:

The makeTwoQubitGateDD constructs the DD bottom-up correctly. 🎉
I have updated the makePeresDD and makePeresInvDD workaround methods because they provided the inverse output instead of the desired one. (i.e. makePeresDD was producing the inverse DD of the Peres gate)
I have updated the code across the code base to rely on the native construction of two-target controlled-gate DDs instead of using the old workaround methods. Additionally, I used them to verify the functionality of our method by comparing the outputs (e.g. compared the output generated by makeTwoQubitGateDD for a swap matrix with makeSWAPDD).
Note: Here there were some already existing methods which were checking this, however, there was only one missing test case for the Peres gate.
To improve the code coverage I have added one test to verify all the edge cases that were not covered by the existing tests. (Update after the CI has finished: the test still needs some improvement, I'll come back with an update for this one)

Apart from point 4 (which is on my TODO list), the only question left from my side is if we still intend to unify makeTwoQubitGateDD and makeGateDD since it was mentioned in #334.

…ontrolled-gate-DDs' into native-two-target-controlled-gate-DDs

burgholzer

Overall this is looking really good already. Many Thanks 🎉
The Python failures should be fixed after #536 is in. Then you can just merge the main branch in or rebase your branch on main.
There still are some warnings left to be fixed and I also added some comments that would be good to address before this gets merged.
Most importantly, the main library code in Package.hpp can be significantly reduced by moving the respective special purpose construction methods to the tests. This should slim down the library quite a bit.

As for your question on combining the two gate DD creation methods: I do not think that is necessary as part of this PR. Content-wise, this PR is great as it is and definitly enough to close the respective issue 👍🏼

include/dd/Operations.hpp

test/dd/test_dd_functionality.cpp

…ontrolled-gate-DDs

test/dd/test_dd_functionality.cpp

+// TEST_F(DDFunctionality, twoTargetControlledGateDDEdgeCases) {
+//   using namespace qc::literals;
+//   auto newNQubits = 12U;
+//   auto newDD = std::make_unique<dd::Package<>>(newNQubits);
+//   auto gate = SWAP;
+//
+//   qc::StandardOperation op, op_neg;
+//   op = qc::StandardOperation(newNQubits, Controls{1, 5, 7, 11}, 3, 9, gate);
+//   op_neg = qc::StandardOperation(newNQubits, Controls{1_nc, 5_nc, 7_nc,
+//   11_nc},
+//                                  3, 9, gate);
+//
+//   ASSERT_NO_THROW(
+//       { e = newDD->multiply(getDD(&op, *newDD), getInverseDD(&op, *newDD));
+//       });
+//   newDD->incRef(e);
+//
+//   ASSERT_NO_THROW({
+//     d = newDD->multiply(getDD(&op_neg, *newDD), getInverseDD(&op_neg,
+//     *newDD));
+//   });
+//   newDD->incRef(d);
+//
+//   // the DD of the identity needs to be reconstructed as well
+//   ident = newDD->makeIdent(newNQubits);
+//
+//   EXPECT_EQ(ident, e);
+//   EXPECT_EQ(ident, d);
+// }


BertiFlorea · 2024-01-22T12:06:41Z

I have just added all the requested changes. The code coverage failed but according to the error message, it seems that it might be a connection problem (I might be wrong still).

burgholzer

Great. Many thanks for another nice contribution! 🎉
I'll just apply some finishing touches and then proceed to merge 🔀

test/dd/test_dd_functionality.cpp

BertiFlorea and others added 2 commits January 16, 2024 19:38

⚡ Added native construction of two-target controlled-gate DDs

8c29e9c

🎨 pre-commit fixes

89a879f

burgholzer assigned BertiFlorea Jan 17, 2024

burgholzer added enhancement New feature or request DD Anything related to the DD package c++ Anything related to C++ code labels Jan 17, 2024

burgholzer added this to the DD Package Improvements milestone Jan 17, 2024

burgholzer requested changes Jan 17, 2024

View reviewed changes

include/dd/Package.hpp Outdated Show resolved Hide resolved

include/dd/Package.hpp Outdated Show resolved Hide resolved

include/dd/Package.hpp Outdated Show resolved Hide resolved

BertiFlorea and others added 6 commits January 17, 2024 12:36

Update include/dd/Package.hpp

56b08c2

Co-authored-by: Lukas Burgholzer <burgholzer@me.com>

Update include/dd/Package.hpp

93363da

Co-authored-by: Lukas Burgholzer <burgholzer@me.com>

🐛 fixed controlled two target gate matrix creation

3931b97

Merge remote-tracking branch 'upstream/main' into native-two-target-c…

c96828a

…ontrolled-gate-DDs

🐛 fixed Peres gate DD methods

114a028

✅ improved code coverage by adding new edge cases and updated old ones

🎨 pre-commit fixes

14378dd

BertiFlorea added 2 commits January 21, 2024 17:00

✅ updated test case for edge cases

8a6ed82

Merge remote-tracking branch 'refs/remotes/origin/native-two-target-c…

b5c2700

…ontrolled-gate-DDs' into native-two-target-controlled-gate-DDs

BertiFlorea requested a review from burgholzer January 21, 2024 16:10

burgholzer requested changes Jan 21, 2024

View reviewed changes

include/dd/Operations.hpp Show resolved Hide resolved

test/dd/test_dd_functionality.cpp Outdated Show resolved Hide resolved

test/dd/test_dd_functionality.cpp Outdated Show resolved Hide resolved

test/dd/test_dd_functionality.cpp Outdated Show resolved Hide resolved

BertiFlorea and others added 3 commits January 22, 2024 08:42

Merge remote-tracking branch 'upstream/main' into native-two-target-c…

5a98392

…ontrolled-gate-DDs

♻️ refactored tests and removed workaround methods from main package

83a34a8

🎨 pre-commit fixes

ce0e950

github-advanced-security bot found potential problems Jan 22, 2024

View reviewed changes

🎨 removed old commented test

d7193ef

BertiFlorea requested a review from burgholzer January 22, 2024 11:57

burgholzer approved these changes Jan 22, 2024

View reviewed changes

burgholzer and others added 2 commits January 22, 2024 13:48

Apply suggestions from code review

4018362

🎨 pre-commit fixes

0ba9f8b

burgholzer enabled auto-merge (squash) January 22, 2024 12:48

burgholzer merged commit d3d74c8 into cda-tum:main Jan 22, 2024
32 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚡ Added native construction of two-target controlled-gate DDs #534

⚡ Added native construction of two-target controlled-gate DDs #534

BertiFlorea commented Jan 16, 2024 •

edited by burgholzer

Loading

codecov bot commented Jan 16, 2024 •

edited

Loading

burgholzer left a comment

BertiFlorea commented Jan 21, 2024

burgholzer left a comment

BertiFlorea commented Jan 22, 2024

burgholzer left a comment

⚡ Added native construction of two-target controlled-gate DDs #534

⚡ Added native construction of two-target controlled-gate DDs #534

Conversation

BertiFlorea commented Jan 16, 2024 • edited by burgholzer Loading

Description

Checklist:

codecov bot commented Jan 16, 2024 • edited Loading

Codecov Report

burgholzer left a comment

Choose a reason for hiding this comment

BertiFlorea commented Jan 21, 2024

burgholzer left a comment

Choose a reason for hiding this comment

BertiFlorea commented Jan 22, 2024

burgholzer left a comment

Choose a reason for hiding this comment

BertiFlorea commented Jan 16, 2024 •

edited by burgholzer

Loading

codecov bot commented Jan 16, 2024 •

edited

Loading