Gather DDSIM benchmarks #12

tyi1025 · 2023-06-14T22:54:04Z

closes #5

codecov · 2023-06-14T23:03:50Z

Codecov Report

Merging #12 (77a1505) into main (fd049a1) will not change coverage.
The diff coverage is 100.0%.

@@           Coverage Diff            @@
##             main      #12    +/-   ##
========================================
  Coverage   100.0%   100.0%            
========================================
  Files          12       28    +16     
  Lines         106      607   +501     
  Branches        0        4     +4     
========================================
+ Hits          106      607   +501

Files Changed	Coverage Δ
include/Executor.hpp	`100.0% <ø> (ø)`
include/executors/CircuitSimulatorExecutor.hpp	`100.0% <ø> (ø)`
include/SimulationExecutor.hpp	`100.0% <100.0%> (ø)`
...nclude/executors/DeterministicNoiseSimExecutor.hpp	`100.0% <100.0%> (ø)`
include/executors/HybridSimulatorExecutor.hpp	`100.0% <100.0%> (ø)`
...ude/executors/StochasticNoiseSimulatorExecutor.hpp	`100.0% <100.0%> (ø)`
include/executors/UnitarySimulatorExecutor.hpp	`100.0% <100.0%> (ø)`
src/executors/CircuitSimulatorExecutor.cpp	`100.0% <100.0%> (ø)`
src/executors/DeterministicNoiseSimExecutor.cpp	`100.0% <100.0%> (ø)`
src/executors/HybridSimulatorExecutor.cpp	`100.0% <100.0%> (ø)`
... and 10 more

... and 1 file with indirect coverage changes

burgholzer

I just did a quick review of the changes here. There are still a couple aspects that definitely need improvement or are missing at the moment. I hope the comments help to get closer towards the goal.
I would assume some of the comments here also directly apply to the other PR. So please also consider them there as applicable.

burgholzer · 2023-07-19T09:16:41Z

benchmarks/CMakeLists.txt

+      CACHE BOOL "" FORCE)
+
+  # store path to googletest in variable
+  set(GTEST_PATH "extern/ddsim/extern/qfr/extern/dd_package/extern/googletest")


Not that the googletest path is subject to change once you rebase the QFR (now MQT Core) PR and the DSSIM/QCEC PRs. Then this becomes: extern/ddsim/extern/mqt-core/extern/googletest

burgholzer · 2023-07-19T09:20:42Z

CMakeLists.txt

+option(BUILD_DD_EVAL_BENCHMARKS "Also build benchmarks for DDEval project")
+if(BUILD_DD_EVAL_BENCHMARKS)
+  enable_testing()
+  include(GoogleTest)
+  add_subdirectory(benchmarks)
+endif()


If you want the cpp-linter to work properly, you also have to set that option in the corresponding CI workflow (.github/workflows/cpp-linter.yaml)

burgholzer · 2023-07-19T09:23:21Z

src/executors/HybridSimulatorDDExecutor.cpp

This executor can just be combined with the other hybrid executor and the simulation type can be specified as an option.
This avoids quite some code duplication.

burgholzer · 2023-07-19T09:24:03Z

src/executors/HybridSimulatorDDExecutor.cpp

+
+  auto qc = std::make_unique<qc::QuantumComputation>(task.getQc()->clone());
+  auto hybridSimulator = std::make_unique<HybridSchrodingerFeynmanSimulator<>>(
+      std::move(qc), ApproximationInfo{}, 23,


Extract the magic seed number as a global constant somewhere so that it can be used everywhere and is just defined in a single place.

burgholzer · 2023-07-19T09:29:36Z

src/executors/StochasticNoiseSimulatorExecutor.cpp

+  auto const constructionStart = std::chrono::steady_clock::now();
+
+  auto qc = std::make_unique<qc::QuantumComputation>(task.getQc()->clone());
+  auto circuitSimulator =
+      std::make_unique<StochasticNoiseSimulator<>>(std::move(qc), 1, 1, 23);
+
+  auto const executionStart = std::chrono::steady_clock::now();
+
+  result["measurement_results"] = circuitSimulator->simulate(1024U);
+  // Add memory usage
+
+  auto const executionStop = std::chrono::steady_clock::now();
+  auto const constructionTime =
+      std::chrono::duration_cast<std::chrono::microseconds>(executionStart -
+                                                            constructionStart);
+  auto const execTime = std::chrono::duration_cast<std::chrono::microseconds>(
+      executionStop - executionStart);
+  result["construction_time"] = constructionTime.count();
+  result["execution_time"]    = execTime.count();
+
+  result["executor"] = getIdentifier();
+  result["task"]     = task.getIdentifier();


A lot of this code throughout these files is repetitive and it should be possible to reduce that code duplication quite considerably.
All of these methods measure the time some construction method takes and the time taken by some execution function. Each of these can be wrapped as a function that takes a function as one of its arguments.
Should remove quite a lot of lines of code.

1e86792
Could you take a look at this commit and see if this way of refactoring is good? If so I'll do it for the rest of the executor classes

That looks good, I think!
It should be possible to move the construction of the circuit to the base class though.
Since the SimulationExecutor is a template class. You will most likely have to define that in the header and make sure that you have the proper includes for all the classes that are used.

burgholzer · 2023-07-19T09:36:56Z

test/test_simexec_berstein.cpp

+// Gives "C++ exception with description "Unsupported non-unitary functionality
+// 'Rst '." thrown in the test body." when run in dynamic


The dynamic versions of the algorithms are only supported by the regular circuit simulator.
Still worth testing them there. And that should also be made easier by the separation by executors.

burgholzer · 2023-07-19T09:37:48Z

test/test_simexec_ghz.cpp

+// Other executors excluded because 128 qubits still have under 1s execution
+// time


Still run these algorithms for all executors to make sure they still work.

burgholzer · 2023-07-19T09:39:05Z

test/test_simexec_simple.cpp

Isn't this file kind of redundant now that there are dedicated test files for the algorithms?

burgholzer · 2023-07-19T09:42:54Z

benchmarks/benchmark_simexec_berstein.cpp

Just commenting on this file, but everything here holds for the other files as well:
Similar to the test, I would split the benchmarks per executor and not by algorithm.

Furthermore, the benchmarks should really test a broad range of qubits, from really small to really large to cover the whole range of circuit sizes and complexity.
It's not enough to just benchmark a handful of circuits here.

Last but not least, this is missing quite some algorithms that are directly available in our libraries. There should be a parity between the algorithms tested in this repo and the algorithms used for benchmarking.

Just to be clear you mean that I should run the same algorithms both for test and benchmark with the only difference being benchmark being more thorough (more tests with different numbers of qubits), right?

Yeah. The tests can just cover certain instances of each benchmark, while the benchmarks should cover a broad range.

burgholzer · 2023-07-19T09:47:35Z

test/test_simexec_berstein.cpp

Just commenting this here since there is not really a better place for that: these test are missing quite some algorithms that are directly available.
This includes the QPE algorithm in its exact and inexact variant, the dynamic versions of the algorithms, the QFT (potentially with non-zero input state), the W-State.

## Description Implement WState from [this](https://github.com/cda-tum/mqt-bench/blob/main/src/mqt/bench/benchmarks/wstate.py) `mqt-bench` script in light of the DD benchmarking in [this PR](tyi1025/dd_eval#12). --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Lukas Burgholzer <[email protected]>

Add seed to CircuitSimulatorExecutor

ac5da11

tyi1025 self-assigned this Jun 14, 2023

tyi1025 linked an issue Jun 14, 2023 that may be closed by this pull request

🧪 Gather DDSIM benchmarks #5

Open

tyi1025 added 8 commits June 15, 2023 01:23

Add general purpose simulators

200ac3c

Add rudimentary tests

d18d2f3

Add quantum algorithm tests

c0c3975

Separate tests and benchmarks

b7c2a84

Split benchmark and test

984c665

Refactor determnoisesimexec and add a test

45bafa1

import clean-up

5d05049

Attempt to fix the CI linter error

9ab6f20

tyi1025 added the enhancement New feature or request label Jul 14, 2023

burgholzer suggested changes Jul 19, 2023

View reviewed changes

tyi1025 added 16 commits July 19, 2023 22:07

Fix CI

dfc92bc

Merge the hybrid executors together to avoid code duplication

bee7e17

Refactor magic number seed into global constant

39885e5

Rename typoed class

d0edbe2

Clean-up

fdadb63

Fix typo

9aec66f

Fix typo

eb183d3

Merge two unitary simulator executors into one

52756e7

Fix cmake

b1f0fe8

Fix coverage

9574955

Refactor to reduce code duplication for two executors

1e86792

Remove redundant unitary executor

206321a

Fix unitary executor

a234b9f

Refactor the rest of the executors for code duplication

668f36d

Move qc construction to the base class

5683b0b

Remove unnecessary comment

3931e6f

tyi1025 added 2 commits August 8, 2023 23:32

Merge branch 'main' into 5-gather-ddsim-benchmarks

9a8472c

Merge branch 'main' into 5-gather-ddsim-benchmarks

f1b0634

tyi1025 mentioned this pull request Aug 8, 2023

Add w state implementation cda-tum/mqt-core#391

Merged

tyi1025 and others added 3 commits August 9, 2023 12:34

DDSIM with WState

2188a03

Added circ sim exec test

3246ec7

Add other simulator executor tests

77a1505

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gather DDSIM benchmarks #12

Gather DDSIM benchmarks #12

tyi1025 commented Jun 14, 2023

codecov bot commented Jun 14, 2023 •

edited

Loading

burgholzer left a comment

burgholzer Jul 19, 2023

burgholzer Jul 19, 2023

burgholzer Jul 19, 2023

burgholzer Jul 19, 2023

burgholzer Jul 19, 2023

tyi1025 Jul 26, 2023

burgholzer Jul 26, 2023

burgholzer Jul 19, 2023

burgholzer Jul 19, 2023

burgholzer Jul 19, 2023

burgholzer Jul 19, 2023

tyi1025 Jul 30, 2023

burgholzer Jul 31, 2023

burgholzer Jul 19, 2023

		// Gives "C++ exception with description "Unsupported non-unitary functionality
		// 'Rst '." thrown in the test body." when run in dynamic

		// Other executors excluded because 128 qubits still have under 1s execution
		// time

Gather DDSIM benchmarks #12

Are you sure you want to change the base?

Gather DDSIM benchmarks #12

Conversation

tyi1025 commented Jun 14, 2023

codecov bot commented Jun 14, 2023 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 14, 2023 •

edited

Loading