-
Notifications
You must be signed in to change notification settings - Fork 29
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
✨ Removal of identity nodes in matrix DDs #358
Conversation
include/dd/Package.hpp
Outdated
if (x.p->isIdentity()) { | ||
if constexpr (n == NEDGE) { | ||
// additionally check if y is the identity in case of matrix | ||
// multiplication | ||
if (y.p->isIdentity()) { | ||
e = makeIdent(start, var); | ||
// e = makeIdent(start, var); |
Check notice
Code scanning / CodeQL
Commented-out code
test/dd/test_package.cpp
Outdated
// TEST(DDPackageTest, expectationValueExceptions) { | ||
// const dd::QubitCount nrQubits = 2; | ||
// | ||
// auto dd = std::make_unique<dd::Package<>>(nrQubits); | ||
// const auto zeroState = | ||
// dd->makeZeroState(static_cast<dd::QubitCount>(nrQubits - 1)); | ||
// const auto xGate = dd->makeGateDD(dd::Xmat, nrQubits, 0); | ||
// | ||
// EXPECT_ANY_THROW(dd->expectationValue(xGate, zeroState)); | ||
// } |
Check notice
Code scanning / CodeQL
Commented-out code
test/dd/test_package.cpp
Outdated
// TEST(DDPackageTest, Ancillaries) { | ||
// auto dd = std::make_unique<dd::Package<>>(4); | ||
// auto hGate = dd->makeGateDD(dd::Hmat, 2, 0); | ||
// auto cxGate = dd->makeGateDD(dd::Xmat, 2, 0_pc, 1); | ||
// auto bellMatrix = dd->multiply(cxGate, hGate); | ||
// | ||
// dd->incRef(bellMatrix); | ||
// auto reducedBellMatrix = | ||
// dd->reduceAncillae(bellMatrix, {false, false, false, false}); | ||
// EXPECT_EQ(bellMatrix, reducedBellMatrix); | ||
// dd->incRef(bellMatrix); | ||
// reducedBellMatrix = | ||
// dd->reduceAncillae(bellMatrix, {false, false, true, true}); | ||
// EXPECT_EQ(bellMatrix, reducedBellMatrix); | ||
// | ||
// auto extendedBellMatrix = dd->extend(bellMatrix, 2); | ||
// dd->incRef(extendedBellMatrix); | ||
// reducedBellMatrix = | ||
// dd->reduceAncillae(extendedBellMatrix, {false, false, true, true}); | ||
// EXPECT_TRUE(reducedBellMatrix.p->e[1].isZeroTerminal()); | ||
// EXPECT_TRUE(reducedBellMatrix.p->e[2].isZeroTerminal()); | ||
// EXPECT_TRUE(reducedBellMatrix.p->e[3].isZeroTerminal()); | ||
// | ||
// EXPECT_EQ(reducedBellMatrix.p->e[0].p->e[0].p, bellMatrix.p); | ||
// EXPECT_TRUE(reducedBellMatrix.p->e[0].p->e[1].isZeroTerminal()); | ||
// EXPECT_TRUE(reducedBellMatrix.p->e[0].p->e[2].isZeroTerminal()); | ||
// EXPECT_TRUE(reducedBellMatrix.p->e[0].p->e[3].isZeroTerminal()); | ||
// | ||
// dd->incRef(extendedBellMatrix); | ||
// reducedBellMatrix = | ||
// dd->reduceAncillae(extendedBellMatrix, {false, false, true, true}, | ||
// false); | ||
// EXPECT_TRUE(reducedBellMatrix.p->e[1].isZeroTerminal()); | ||
// EXPECT_TRUE(reducedBellMatrix.p->e[2].isZeroTerminal()); | ||
// EXPECT_TRUE(reducedBellMatrix.p->e[3].isZeroTerminal()); | ||
// | ||
// EXPECT_EQ(reducedBellMatrix.p->e[0].p->e[0].p, bellMatrix.p); | ||
// EXPECT_TRUE(reducedBellMatrix.p->e[0].p->e[1].isZeroTerminal()); | ||
// EXPECT_TRUE(reducedBellMatrix.p->e[0].p->e[2].isZeroTerminal()); | ||
// EXPECT_TRUE(reducedBellMatrix.p->e[0].p->e[3].isZeroTerminal()); | ||
// } |
Check notice
Code scanning / CodeQL
Commented-out code
test/dd/test_package.cpp
Outdated
// TEST(DDPackageTest, Identity) { | ||
// auto dd = std::make_unique<dd::Package<>>(4); | ||
// | ||
// EXPECT_TRUE(dd->makeIdent(0).isOneTerminal()); | ||
// EXPECT_TRUE(dd->makeIdent(0, -1).isOneTerminal()); | ||
// | ||
// auto id3 = dd->makeIdent(3); | ||
// EXPECT_EQ(dd->makeIdent(0, 2), id3); | ||
// const auto& table = dd->getIdentityTable(); | ||
// EXPECT_NE(table[2].p, nullptr); | ||
// | ||
// auto id2 = dd->makeIdent(0, 1); // should be found in idTable | ||
// EXPECT_EQ(dd->makeIdent(2), id2); | ||
// | ||
// auto id4 = dd->makeIdent(0, 3); // should use id3 and extend it | ||
// EXPECT_EQ(dd->makeIdent(0, 3), id4); | ||
// EXPECT_NE(table[3].p, nullptr); | ||
// | ||
// auto idCached = dd->makeIdent(4); | ||
// EXPECT_EQ(id4, idCached); | ||
// } |
Check notice
Code scanning / CodeQL
Commented-out code
test/dd/test_package.cpp
Outdated
// TEST(DDPackageTest, Extend) { | ||
// auto dd = std::make_unique<dd::Package<>>(4); | ||
// | ||
// auto id = dd->makeIdent(3); | ||
// EXPECT_EQ(id.p->v, 2); | ||
// EXPECT_EQ(id.p->e[0], id.p->e[3]); | ||
// EXPECT_EQ(id.p->e[1], id.p->e[2]); | ||
// EXPECT_TRUE(id.p->isIdentity()); | ||
// | ||
// auto ext = dd->extend(id, 0, 1); | ||
// EXPECT_EQ(ext.p->v, 3); | ||
// EXPECT_EQ(ext.p->e[0], ext.p->e[3]); | ||
// EXPECT_EQ(ext.p->e[1], ext.p->e[2]); | ||
// EXPECT_TRUE(ext.p->isIdentity()); | ||
// } |
Check notice
Code scanning / CodeQL
Commented-out code
test/dd/test_package.cpp
Outdated
//auto goalRow0 = | ||
// dd::CVec{{1., 0.}, {0., 0.}}; | ||
//auto goalRow1 = | ||
// dd::CVec{{0., 0.}, {1., 0.}}; | ||
//auto goalMatrix = dd::CMat{{1, 0}, {0, 1}};//dd::CMat{goalRow0, goalRow1}; | ||
//ASSERT_EQ(dd->getMatrix(cxGate), goalMatrix); |
Check notice
Code scanning / CodeQL
Commented-out code
test/dd/test_package.cpp
Outdated
dd::CVec{{0., 0.}, {0., 0.}, {1., 0.}, {0., 0.}}; | ||
auto goalRow3 = | ||
dd::CVec{{0., 0.}, {0., 0.}, {0., 0.}, {1., 0.}}; | ||
auto goalMatrix = dd::CMat{goalRow0, goalRow1, goalRow2, goalRow3};//dd::CMat{goalRow0, goalRow1}; |
Check notice
Code scanning / CodeQL
Commented-out code
include/dd/Package.hpp
Outdated
// process lines below target | ||
auto z = static_cast<Qubit>(start); | ||
std::array<mEdge, NEDGE> local{}; | ||
for (; z < target; z++) { |
Check failure
Code scanning / CodeQL
Comparison of narrow type with wide type in loop condition
include/dd/Package.hpp
Outdated
// ComplexValue getValueByPath(const mEdge& e, const Complex& amp, std::size_t | ||
// i, | ||
// std::size_t j, int level) { | ||
// auto c = cn.mulCached(e.w, amp); | ||
// | ||
// if (e.isTerminal()) { | ||
// cn.returnToCache(c); | ||
// return {CTEntry::val(c.r), CTEntry::val(c.i)}; | ||
// } | ||
// | ||
// const bool row = (i & (1ULL << level)) != 0U; | ||
// const bool col = (j & (1ULL << level)) != 0U; | ||
// | ||
// ComplexValue r{}; | ||
// if (!row && !col && !e.p->e[0].w.approximatelyZero()) { | ||
// r = getValueByPath(e.p->e[0], c, i, j, level-1); | ||
// } else if (!row && col && !e.p->e[1].w.approximatelyZero()) { | ||
// r = getValueByPath(e.p->e[1], c, i, j, level-1); | ||
// } else if (row && !col && !e.p->e[2].w.approximatelyZero()) { | ||
// r = getValueByPath(e.p->e[2], c, i, j, level-1); | ||
// } else if (row && col && !e.p->e[3].w.approximatelyZero()) { | ||
// r = getValueByPath(e.p->e[3], c, i, j, level-1); | ||
// } | ||
// cn.returnToCache(c); | ||
// return r; | ||
//} |
Check notice
Code scanning / CodeQL
Commented-out code
include/dd/Package.hpp
Outdated
// ComplexValue getValueByPath(const vEdge& e, const Complex& amp, | ||
// std::size_t i) { | ||
// auto c = cn.mulCached(e.w, amp); | ||
// | ||
// if (e.isTerminal()) { | ||
// cn.returnToCache(c); | ||
// return {CTEntry::val(c.r), CTEntry::val(c.i)}; | ||
// } | ||
// | ||
// const bool one = (i & (1ULL << e.p->v)) != 0U; | ||
// | ||
// ComplexValue r{}; | ||
// if (!one && !e.p->e[0].w.approximatelyZero()) { | ||
// r = getValueByPath(e.p->e[0], c, i); | ||
// } else if (one && !e.p->e[1].w.approximatelyZero()) { | ||
// r = getValueByPath(e.p->e[1], c, i); | ||
// } | ||
// cn.returnToCache(c); | ||
// return r; | ||
//} |
Check notice
Code scanning / CodeQL
Commented-out code
test/dd/test_package.cpp
Outdated
// auto goalRow0 = | ||
// dd::CVec{{1., 0.}, {0., 0.}}; | ||
// auto goalRow1 = | ||
// dd::CVec{{0., 0.}, {1., 0.}}; | ||
// auto goalMatrix = dd::CMat{{1, 0}, {0, 1}};//dd::CMat{goalRow0, goalRow1}; | ||
// ASSERT_EQ(dd->getMatrix(cxGate), goalMatrix); |
Check notice
Code scanning / CodeQL
Commented-out code
test/dd/test_package.cpp
Outdated
auto goalRow2 = dd::CVec{{0., 0.}, {0., 0.}, {1., 0.}, {0., 0.}}; | ||
auto goalRow3 = dd::CVec{{0., 0.}, {0., 0.}, {0., 0.}, {1., 0.}}; | ||
auto goalMatrix = dd::CMat{goalRow0, goalRow1, goalRow2, | ||
goalRow3}; // dd::CMat{goalRow0, goalRow1}; |
Check notice
Code scanning / CodeQL
Commented-out code
test/dd/test_package.cpp
Outdated
// TEST(DDPackageTest, ToffoliTable) { | ||
// auto dd = std::make_unique<dd::Package<>>(4); | ||
// | ||
// // try to search for a toffoli in an empty table | ||
// auto toffoli = dd->toffoliTable.lookup(3, {0_nc, 1_pc}, 2); | ||
// EXPECT_EQ(toffoli.p, nullptr); | ||
// if (toffoli.p == nullptr) { | ||
// toffoli = dd->makeGateDD(dd::Xmat, 3, {0_nc, 1_pc}, 2); | ||
// dd->toffoliTable.insert(3, {0_nc, 1_pc}, 2, toffoli); | ||
// } | ||
// | ||
// // try again with same toffoli | ||
// auto toffoliTableEntry = dd->toffoliTable.lookup(3, {0_nc, 1_pc}, 2); | ||
// EXPECT_NE(toffoliTableEntry.p, nullptr); | ||
// EXPECT_EQ(toffoli, toffoliTableEntry); | ||
// | ||
// // try again with different controlled toffoli | ||
// toffoliTableEntry = dd->toffoliTable.lookup(3, {0_pc, 1_pc}, 2); | ||
// EXPECT_EQ(toffoliTableEntry.p, nullptr); | ||
// | ||
// // try again with different qubit toffoli | ||
// toffoliTableEntry = dd->toffoliTable.lookup(4, {0_pc, 1_pc, 2_pc}, 3); | ||
// EXPECT_EQ(toffoliTableEntry.p, nullptr); | ||
// | ||
// // clear the table | ||
// dd->toffoliTable.clear(); | ||
// toffoliTableEntry = dd->toffoliTable.lookup(3, {0_nc, 1_pc}, 2); | ||
// EXPECT_EQ(toffoliTableEntry.p, nullptr); | ||
// } |
Check notice
Code scanning / CodeQL
Commented-out code
include/dd/Package.hpp
Outdated
@@ -544,7 +563,9 @@ | |||
|
|||
[[maybe_unused]] const auto before = cn.cacheCount(); | |||
|
|||
const auto level = static_cast<Qubit>(std::log2(length) - 1); | |||
if (level == -1) { |
Check warning
Code scanning / CodeQL
Comparison result is always the same
include/dd/Package.hpp
Outdated
getMatrix(e.p->e[0], c, i, j, mat, level - 1); | ||
} | ||
if (!e.p->e[1].w.approximatelyZero()) { | ||
getMatrix(e.p->e[1], c, i, y, mat, level - 1); |
Check failure
Code scanning / CodeQL
Potentially uninitialized local variable
include/dd/Package.hpp
Outdated
getMatrix(e.p->e[1], c, i, y, mat, level - 1); | ||
} | ||
if (!e.p->e[2].w.approximatelyZero()) { | ||
getMatrix(e.p->e[2], c, x, j, mat, level - 1); |
Check failure
Code scanning / CodeQL
Potentially uninitialized local variable
include/dd/Package.hpp
Outdated
getMatrix(e.p->e[2], c, x, j, mat, level - 1); | ||
} | ||
if (!e.p->e[3].w.approximatelyZero()) { | ||
getMatrix(e.p->e[3], c, x, y, mat, level - 1); |
Check failure
Code scanning / CodeQL
Potentially uninitialized local variable
include/dd/Package.hpp
Outdated
getMatrix(e.p->e[2], c, x, j, mat, level - 1); | ||
} | ||
if (!e.p->e[3].w.approximatelyZero()) { | ||
getMatrix(e.p->e[3], c, x, y, mat, level - 1); |
Check failure
Code scanning / CodeQL
Potentially uninitialized local variable
include/dd/Package.hpp
Outdated
} else if ((!e.isTerminal() && e.p->v < level) || | ||
(e.isTerminal() && level != -1)) { | ||
getMatrix(e, c, i, j, mat, level - 1); | ||
getMatrix(e, c, x, y, mat, level - 1); |
Check failure
Code scanning / CodeQL
Potentially uninitialized local variable
include/dd/Package.hpp
Outdated
} else if ((!e.isTerminal() && e.p->v < level) || | ||
(e.isTerminal() && level != -1)) { | ||
getMatrix(e, c, i, j, mat, level - 1); | ||
getMatrix(e, c, x, y, mat, level - 1); |
Check failure
Code scanning / CodeQL
Potentially uninitialized local variable
include/dd/Simulation.hpp
Outdated
@@ -26,7 +26,7 @@ | |||
|
|||
// correct permutation if necessary | |||
changePermutation(e, permutation, qc->outputPermutation, dd); | |||
e = dd->reduceGarbage(e, qc->garbage); | |||
// e = dd->reduceGarbage(e, qc->garbage); |
Check notice
Code scanning / CodeQL
Commented-out code
src/dd/Simulation.cpp
Outdated
@@ -83,7 +83,7 @@ | |||
|
|||
// correct permutation if necessary | |||
changePermutation(e, permutation, qc->outputPermutation, dd); | |||
e = dd->reduceGarbage(e, qc->garbage); | |||
// e = dd->reduceGarbage(e, qc->garbage); |
Check notice
Code scanning / CodeQL
Commented-out code
Signed-off-by: burgholzer <[email protected]>
59866e2
to
d5993fc
Compare
Hey 👋🏼
Additionally, in There are still many issues and changes that need to be addressed, but for the moment if you could take a look and review the current status would be great! (I am sure there are flagrant mistakes or some urgent changes that need to be done in order to also fix the CI) Any hint or advice no matter how small is very welcome! 🙂 Lastly, apart from bringing the branch up to date, as discussed, there are some methods in the package that do not yet account for the missing identity nodes. Would it be possible to highlight them in your review to make it easier to track? 🙏🏼 P.S. Many thanks in advance for your time to review, because I know there are quite a lot of places to investigate. (I think around 211 commits have been merged) |
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.
Hey @BertiFlorea 👋🏼
Many thanks for taking on this task and resolving all those conflicts with main
. That must have been quite burdensome.
Nevertheless, I think the merge went rather well. I added comments to some leftover places, where something is not quite right yet. But these should be easy to fix.
Naturally, the most important thing right now is getting the code to compile again.
After that, some of the methods will need adjustments to account for the new identity-less structure.
The good thing here is that the gate construction, the addition, and the multiplication all seem to be in a good state already and look really clean now. This means that the main functionality should be working well already.
All it takes now is to "just" (that's a big "just") adapt the utility functions for translating DDs to vectors and matrices as well as some smaller mathematical functions such as the kronecker product and ancillary/garbage reduction.
As you requested, I explicitly tried to add comments to the respective places that should help keeping track of what still needs to change.
We can really take this step. So let's try to first resolve all the easy comments and suggestions.
Let me know if you have any questions.
Co-authored-by: Lukas Burgholzer <[email protected]>
Co-authored-by: Lukas Burgholzer <[email protected]>
Co-authored-by: Lukas Burgholzer <[email protected]>
Co-authored-by: Lukas Burgholzer <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #358 +/- ##
=====================================
Coverage 91.1% 91.2%
=====================================
Files 132 132
Lines 13918 13998 +80
Branches 2197 2230 +33
=====================================
+ Hits 12693 12775 +82
+ Misses 1225 1223 -2
|
Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
# Conflicts: # test/dd/test_package.cpp
## Description This PR fixes #567 and should subsequently allow to fix cda-tum/mqt-ddsim#361, which is the final blocker for merging #358. ## 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. - [x] 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. --------- Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
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.
All the checks are green, all the dependent PRs are green as well.
So this epic can finally be merged 🎉
Thanks to everyone that contributed!
## Description This PR pulls in the changes from cda-tum/mqt-core#358, which considerably changes the way matrix decision diagrams are represented. Particularly, any node resembling the identity is now eliminated and only implicitly represented. This further compacts the representation of quantum gates and makes the identity the most compact it can be---a single terminal node. The overall performance improvements are still to be evaluated. ## 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. - [x] 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. --------- Signed-off-by: burgholzer <[email protected]>
## Description This is the first of probably many follow-up PRs to #358, which tries to simplify the underlying functions and methods for representing operations. This PR specifically streamlines the `makeGateDD` method in the DD package and eliminates the `nqubits` and `start` parameters, which are no longer necessary. ## 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. - [x] 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. Signed-off-by: burgholzer <[email protected]>
## Description The second follow-up PRs to #358, which tries to simplify the underlying functions and methods for representing operations. This PR specifically streamlines the `makeTwoQubitGateDD` method in the DD package and eliminates the `nqubits` and `start` parameters, which are no longer necessary. ## 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. - [x] 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. --------- Signed-off-by: burgholzer <[email protected]>
…rarchy (#574) ## Description The third follow-up PRs to #358, which tries to simplify the `Operation` class hierarchy by eliminating the `nqubits` and `startingQubit` members, which were purely in place for managing the corresponding decision diagrams. This also practically fixes #345. Only state creation routines now have the additional starting qubit parameter. ## 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. - [x] 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. --------- Signed-off-by: burgholzer <[email protected]>
## Description This PR pulls in the changes from cda-tum/mqt-core#358, which considerably changes the way matrix decision diagrams are represented. Particularly, any node resembling the identity is now eliminated and only implicitly represented. This further compacts the representation of quantum gates and makes the identity the most compact it can be---a single terminal node. The overall performance improvements are still to be evaluated. Surprisingly few changes were needed to make this work in QCEC. ## 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. - [x] 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. --------- Signed-off-by: burgholzer <[email protected]>
This pull request strips matrix DDs of their identities, representing them implicitly instead of explicitly.
This includes not constructing a gate DD for the full system size (i.e. a single qubit operation is expanded to n-qubit with identities), skipping nodes that resemble the identity, as well as handling skipped levels in operations such as multiplication and addition.
The identity is now represented by a single terminal node.