✨ Migrate and Refactor Approximation Functionality from DDSim Package #908

MatthiasReumann · 2025-04-06T15:56:07Z

Description

This PR adds DD approximation functionality. Most of the functions originate from the DDSim Package. However, instead of simply copying and gluing the packages together, I re-implemented and refactored most of it.

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 · 2025-04-07T19:11:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

burgholzer

Just a couple of thoughts because I skimmed over the PR 😌
I also approved the CI so that the corresponding checks should run now.

src/dd/Simulation.cpp

include/mqt-core/dd/Simulation.hpp

include/mqt-core/dd/Approximation.hpp

…llect

test/dd/test_approximations.cpp

src/dd/Approximation.cpp

github-advanced-security

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

burgholzer

I really really like where this is going. Thanks for your continued work on this.
The structure of the approximation has already become way cleaner and clearer.
As you will see from the comments, there are a couple of complexity aspects that are still suboptimal and where I believe improvements make sense.
Most of these should be fairly easy to incorporate. The hardest one will probably be avoiding the two-stage approximation traversal. But, at the same time, it's also the most drastic performance blocker.

I would be tempted to say that some tests that track the complexity of the approximation would be in order, but I know from experience that these are near impossible to write. Or they require some kind of additional tracking, which results in an overhead memory as well as runtime wise.
Some kind of middle ground could be to run the tests of the test suite through a profile and check the call counts for certain critical functions. This would probably reveal the exponential complexity of the current rebuild function.
Maybe these thoughts trigger an idea on your side for how to further improve the tests (which are already really great!)

burgholzer · 2025-04-30T19:46:59Z

test/dd/test_approximations.cpp

+
+  // Test: If the budget is 0, no approximation will be applied.
+  //
+  // |state⟩ = 0.866|0⟩ + 0.5|1⟩


That's not the state created by this test circuit😉

I suspect you might want to change the circuit.

thanks & sorry🤦🏼

burgholzer · 2025-04-30T19:50:31Z

test/dd/test_approximations.cpp

+  auto p = approximate(state, fidelity, *dd);
+  auto approx = p.first;


Probably just a matter of taste, but this could look a little cleaner using structured bindings and an additional assertion that the returned fidelity is 1.
Something like

const auto& [approx, fid] = approximate(state, fidelity, *dd); EXPECT_EQ(fid, fidelity);

Similar reasoning can be applied to the other tests.

burgholzer · 2025-04-30T19:51:53Z

test/dd/test_approximations.cpp

+
+  // Test: If the budget is too small, no approximation will be applied.
+  //
+  // |state⟩ = 0.866|0⟩ + 0.5|1⟩


Also not the state being prepared in the test.

burgholzer · 2025-04-30T19:54:52Z

test/dd/test_approximations.cpp

+
+  dd->decRef(p.first);
+  dd->garbageCollect(true);
+
+  EXPECT_EQ(dd->vUniqueTable.getNumEntries(), 0);


I'd argue that this could be combined with the previous test.
The description comment is still helpful and could also be copied over.

burgholzer · 2025-04-30T20:00:24Z

test/dd/test_approximations.cpp

+  qcRef.h(0);
+  qcRef.x(1);
+  qcRef.cx(0, 1);
+  qcRef.cx(1, 0);


Maybe I am stupid, but the resulting state is a product state, so single-qubit operations are completely sufficient to construct the reference state.
Specifically,

Suggested change

qcRef.h(0);

qcRef.x(1);

qcRef.cx(0, 1);

qcRef.cx(1, 0);

qcRef.x(0);

qcRef.h(1);

Should just work.

You are absolutely right.

burgholzer · 2025-04-30T20:33:26Z

src/dd/Approximation.cpp

+      if (contribution <= budget) {
+        exclude.emplace_front(e);
+        budget -= contribution;
+      } else if (!e->isTerminal()) {
+        const vNode* n = e->p;
+
+        for (const vEdge& eChildRef : n->e) {
+          const vEdge* eChild = &eChildRef;
+
+          if (!eChild->w.exactlyZero()) {
+            const double childContribution =
+                contribution * ComplexNumbers::mag2(eChild->w);
+            const auto it = std::find_if(
+                next.begin(), next.end(),
+                [&eChild](const auto& p) { return p.first == eChild; });
+            if (it == next.end()) {
+              next.emplace_front(eChild, childContribution);
+            } else {
+              (*it).second += childContribution;
+            }
+          }
+        }
+      }


This might just be me and my personal programming preference, so there is definitely no need for you to change that, but just so that I have said it.

I am a big fan of reducing the levels of nesting in code.
I would probably place a continue statement in the first if block and de-nest the subsequent else if, by flipping the if condition and continuing if the condition is not met.
That reduces at least one level of indirection, if not two.
At times, this can really help to make the code a little cleaner and seam less complex.
The exactlyZero check could also be flipped to de-nest the subsequent code.

burgholzer · 2025-04-30T21:49:47Z

src/dd/Approximation.cpp

+            const auto it = std::find_if(
+                next.begin(), next.end(),
+                [&eChild](const auto& p) { return p.first == eChild; });


Thinking about the complexity here, I believe this could also benefit from using an unordered_set.
Iteration speed is similar. Lookup is way faster.
Should be a no brainer.

burgholzer · 2025-04-30T21:55:52Z

src/dd/Approximation.cpp

+  }
+
+  VectorDD approx = rebuild(state, exclude, dd);
+  approx.w = dd.cn.lookup(approx.w / std::sqrt(ComplexNumbers::mag2(approx.w)));


I was about to write that this computation is likely to be redundant because you could simply set the top weight to one, but that would discard the global phase.

What could work though:
Before starting the approximation, safe the top edge weight of the incoming DD.
Then set the weight to exactly one and start the approximation procedure.
At the end, set approx.w to the initial edge weight.

I am fairly certain that the top edge weight is not affected by approximation, so the above simplification should be valid.

burgholzer · 2025-04-30T21:59:31Z

include/mqt-core/dd/Approximation.hpp

+ * @details Traverses the decision diagram layer by layer in a breadth-first
+ * manner (iterative deepening algorithm) and eliminates edges greedily until
+ * the budget (1 - @p fidelity) is exhausted.


Once all the details of the implementation are clarified, I believe it would be helpful to add a brief description of the complexity of the approximation operation to the docstring here.

burgholzer · 2025-04-30T22:12:02Z

src/dd/Approximation.cpp

+ * @brief Recursively rebuild @p state. Exclude edges contained in @p exclude.
+ * @return Rebuilt VectorDD.
+ */
+VectorDD rebuild(const VectorDD& state,


At the moment, this method traverses the full DD down to the terminals. And it does so without checking whether a certain successor has already been processed. As such, this function will necessarily scale exponentially in the number of qubits since it will visit every possible path through the decision diagram (which, in a decision diagram without zero terminals, are always exponentially many).
This is undesirable and seems unnecessary.

Furthermore, with this function, there is a second top-down traversal of the decision diagram in the approximation routine. One for collecting the nodes to be eliminated, and another one for eliminating the nodes themselves. I am fairly certain that there is a way to directly eliminate the nodes as part of the main descent through the decision diagram.
This might not be as apparent because the main approximation loop is implemented iteratively instead of recursively (like this function is).
However, I think this can be made to work even in the iterative case.
This would also avoid one of the drawbacks here that the current method really rebuilds the entire DD; even when there is nothing to eliminate.
Just as an example for a potential optimization (which will probably be obsolete if this two-step pass is avoided), if I look at the set of edges to be excluded an none of them has a variable index smaller than X, then I do not need to descend further than to level X+1 because nothing will change below that level.

burgholzer · 2025-04-30T22:19:43Z

Ah, and don't be worried about the failing Python tests in the CI. These are unrelated to your changes and will be more or less automatically resolved soon 😌

MatthiasReumann added 7 commits April 1, 2025 09:56

Add Approximation.hpp

804f39b

Add NodeContributions struct to Approximation.hpp

454d5bc

Update vectorize function to iterate breadth-first

afb6b6f

Add Approximation to simulate function

182b48a

Slim down NodeContributions class

2a7b5cd

Implement applyApproximation

080774b

Add preliminary unit tests

43b9569

github-project-automation bot added this to MQT Core Apr 6, 2025

github-project-automation bot moved this to In Progress in MQT Core Apr 6, 2025

pre-commit-ci bot and others added 7 commits April 6, 2025 15:56

🎨 pre-commit fixes

82580a7

Merge branch 'main' into feat/dd-sim-approximations

3919879

Update doxygen for simulate function

1f841ad

Make Approximation structs constexpr

22c4e55

Show in code that MemoryDriven is FidelityDriven + threshold

1d3a7e5

Add rebuildWithout function

472958c

Merge branch 'main' into feat/dd-sim-approximations

017628b

burgholzer added refactor Anything related to code refactoring feature New feature or request DD Anything related to the DD package c++ Anything related to C++ code labels Apr 7, 2025

burgholzer added this to the DD Package Improvements milestone Apr 7, 2025

burgholzer reviewed Apr 7, 2025

View reviewed changes

src/dd/Simulation.cpp Outdated Show resolved Hide resolved

src/dd/Simulation.cpp Outdated Show resolved Hide resolved

include/mqt-core/dd/Simulation.hpp Outdated Show resolved Hide resolved

include/mqt-core/dd/Approximation.hpp Show resolved Hide resolved

MatthiasReumann added 5 commits April 8, 2025 08:13

Remove faulty lookup

5dbef62

Add renormalization to root edge

4a6a156

Merge branch 'main' into feat/dd-sim-approximations

14eb307

1-traversal approximation

378800a

Apply correct edge weights and update tests

0c77295

github-project-automation bot added this to MQT Core Apr 12, 2025

Fix includes

7dce932

MatthiasReumann added 4 commits April 28, 2025 10:03

Add std::find for nextEdge

1c8c556

Merge branch 'main' into feat/dd-sim-approximations

5c8489e

Remove checks with pre-approximation state after decRef and garbageCo…

6b17acf

…llect

Update doxygen

25f7de9

github-advanced-security bot found potential problems Apr 28, 2025

View reviewed changes

test/dd/test_approximations.cpp Fixed Show fixed Hide fixed

Remove unused postFidelity in tests

84a9f06

github-advanced-security bot found potential problems Apr 28, 2025

View reviewed changes

src/dd/Approximation.cpp Fixed Show fixed Hide fixed

Comment out rebuild function

a8dc8b9

github-advanced-security bot found potential problems Apr 28, 2025

View reviewed changes

src/dd/Approximation.cpp Fixed Show fixed Hide fixed

src/dd/Approximation.cpp Fixed Show fixed Hide fixed

src/dd/Approximation.cpp Fixed Show fixed Hide fixed

MatthiasReumann added 2 commits April 28, 2025 16:38

Uncomment contribution map

ecdb253

Comment all but simplest test

3b234b7

github-advanced-security bot found potential problems Apr 29, 2025

View reviewed changes

MatthiasReumann added 8 commits April 29, 2025 06:49

Comment out approximate function in simplest test

a7cc5fd

Initialize contributions map value explicitly

4fa3524

Comment approximate function

e642827

Uncomment code

ab939de

Use unique_ptr for dd::Package

90bb242

Eliminate contribution map

ae89cdf

Remove unused import of unordered_map

f95f388

Formatting

4e638b7

MatthiasReumann marked this pull request as ready for review April 30, 2025 06:42

burgholzer requested changes Apr 30, 2025

View reviewed changes

MatthiasReumann added 7 commits May 1, 2025 07:43

Update tests, its descriptions, and add ascii dds

7fa8900

Remove 0. in dd vis

6015b69

De-nest if statements

42ac8df

Extract global phase

9d79391

Use unordered_map for layer

4a8d67b

Remove edges without a second top-down traversal

5676056

Merge branch 'main' into feat/dd-sim-approximations

bf5d09c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Migrate and Refactor Approximation Functionality from DDSim Package #908

✨ Migrate and Refactor Approximation Functionality from DDSim Package #908

MatthiasReumann commented Apr 6, 2025 •

edited

Loading

codecov bot commented Apr 7, 2025 •

edited

Loading

burgholzer left a comment

github-advanced-security bot left a comment

burgholzer left a comment

burgholzer Apr 30, 2025

MatthiasReumann May 1, 2025

burgholzer Apr 30, 2025

burgholzer Apr 30, 2025

burgholzer Apr 30, 2025

burgholzer Apr 30, 2025

MatthiasReumann May 1, 2025

burgholzer Apr 30, 2025

burgholzer Apr 30, 2025

burgholzer Apr 30, 2025

burgholzer Apr 30, 2025

burgholzer Apr 30, 2025

burgholzer commented Apr 30, 2025

		auto p = approximate(state, fidelity, *dd);
		auto approx = p.first;

✨ Migrate and Refactor Approximation Functionality from DDSim Package #908

Are you sure you want to change the base?

✨ Migrate and Refactor Approximation Functionality from DDSim Package #908

Conversation

MatthiasReumann commented Apr 6, 2025 • edited Loading

Description

Checklist:

codecov bot commented Apr 7, 2025 • edited Loading

Codecov Report

burgholzer left a comment

Choose a reason for hiding this comment

github-advanced-security bot left a comment

Choose a reason for hiding this comment

burgholzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burgholzer commented Apr 30, 2025

MatthiasReumann commented Apr 6, 2025 •

edited

Loading

codecov bot commented Apr 7, 2025 •

edited

Loading