Sparse simulator #767

sam-jaques · 2021-07-05T18:00:01Z

This PR adds a new simulator that uses memory-efficient representations of sparse quantum states.

ghost · 2021-07-05T18:00:15Z

All CLA requirements met.

src/Simulation/Simulators/SparseSimulator/README.md

src/Simulation/Simulators/SparseSimulator/Native/SparseSimulator.h

src/Simulation/Simulators/SparseSimulator/Native/gates.h

kuzminrobin · 2021-07-06T23:15:16Z

src/Simulation/Simulators/SparseSimulator/Native/gates.h

+                logical_qubit_id shift;
+                logical_qubit_id target_2;


FYI: A number of data members stay uninitialized in a number of constructors.
You may find useful the Non-static data member initializers.

kuzminrobin · 2021-07-06T23:35:51Z

src/Simulation/Simulators/SparseSimulator/Native/SparseSimulator.h

@@ -0,0 +1,846 @@
+#pragma once


(Up to you)
This is a C++ file.

You may want to change the extension from .h to .hpp.
You may want to break up the file into .hpp and .cpp.

src/Simulation/Simulators/SparseSimulator/Native/SparseSimulator.h

cgranade

Thanks for opening this and for your contribution, @sam-jaques! I took a quick look through and left some comments, but happy to expand on anything that would be helpful. I think my overall feedback, though, is that it would be good to integrate this into the build pipelines and to make sure that your new simulator could be used from Python.

cgranade · 2021-07-05T18:14:59Z

src/Simulation/Simulators/SparseSimulator/SparseSimulatorCS/Probes.cs

+                return QVoid.Instance;
+            }
+
+            // This generates the channel, then calls it


In order to be intercepted by IQ# and Python hosts, this dump needs to be written out to SimulatorBase.OnDisplayableDiagnostic rather than to Message.

src/Simulation/Simulators/SparseSimulator/SparseSimulatorCS/Probes.cs

cgranade · 2021-07-05T18:15:46Z

src/Simulation/Simulators/SparseSimulator/SparseSimulatorCS/Probes.cs

+        /// <summary>
+        /// Returns a string that represents the phase of the amplitude.
+        /// </summary>
+        private string FormatAngle(double magnitude, double angle)


It looks like this may be duplicated with other angle formatting functionality.

src/Simulation/Simulators/SparseSimulator/Native/SparseSimulator.h

src/Simulation/Simulators/SparseSimulator/Native/CMakeLists.txt

cgranade · 2021-07-06T17:17:25Z

src/Simulation/Simulators/SparseSimulator/SparseSimulatorTests/TestHelpers.cpp

+		return mask;
+	}
+	// power of square root of -1
+	inline amplitude iExp(int power)


It looks like you have a mix of snake_case and camelCase here?

cgranade · 2021-07-06T17:19:06Z

src/Simulation/Simulators/SparseSimulator/SparseSimulatorTests/pch.h

+#ifndef PCH_H
+#define PCH_H
+
+// add headers that you want to pre-compile here


Is this file needed? It looks like there's no headers nominated for precompilation.

cgranade · 2021-07-06T17:20:05Z

src/Simulation/Simulators/SparseSimulator/build.cake

+var release_configuration = Argument("configuration", "Release");
+var debug_configuration = Argument("configuration", "Debug");


I'd recomend camelCase instead of snake_case here.

cgranade · 2021-07-06T22:58:51Z

src/Simulation/Simulators/SparseSimulator/Native/capi.cpp

+#include<chrono>
+#include<thread>
+
+std::string sample_string;


This definition is pretty far from where it's used; a comment or such would be helpful here. More to the point, though, it looks like there's a single std::string shared between each simulator— what happens if multiple simulators are being run in different threads, with each running sample at the same time?

This isn't as contrived as it may sound, as that kind of issue can easily arise when running unit tests. Many test harnesses will parallelize by default, such that guarding shared resources with a mutex or other kind of lock may be necessary.

It looks like _mutex guards the vector of simulators, but once a simulator is extracted from the vector, a race condition may still be possible?

src/Simulation/Simulators/SparseSimulator/Native/SparseSimulator.h

kuzminrobin · 2021-07-07T00:32:58Z

src/Simulation/Simulators/SparseSimulator/Native/factory.cpp

+    }
+    else
+    {
+        _simulators[emptySlot] = std::shared_ptr<SparseSimulator>(new SparseSimulator(num_qubits));


For the future FYI: Item 21: Prefer std::make_unique and std::make_shared to direct use of new.

kuzminrobin · 2021-07-07T00:43:16Z

src/Simulation/Simulators/SparseSimulator/Native/factory.cpp

+std::shared_ptr<SparseSimulator>& getSimulator(unsigned id)
+{
+    std::shared_lock<std::shared_mutex> shared_lock(_mutex);
+
+    return _simulators[id];
+}


FYI: Returns a non-const reference to the element of a container, thus enabling the update of the element in a non-thread-safe manner. E.g. someone can call getSimulator(..).reset() (the reset() call is not protected with _mutex) while some other thread can be accessing that same element (access protected with _mutex).

Consider returning by value:
std::shared_ptr<SparseSimulator> getSimulator(unsigned id) (no ampersand &).

kuzminrobin · 2021-07-07T00:47:06Z

src/Simulation/Simulators/SparseSimulator/Native/capi.cpp

+#include<chrono>
+#include<thread>
+
+std::string sample_string;


Consider making sample_string a local variable (and a const reference to std::string). It is accessed in one function only.

bettinaheim · 2021-07-07T18:16:09Z

@sam-jaques Thanks for putting this together!
I'll create a feature branch shortly for this to be merged into, and @DmitryVasilevsky will coordinate and work with you on updating this branch with the latest and processing the feedback.

kuzminrobin · 2021-07-07T18:47:24Z

I made a quick run through the C/C++ code (but not C# code). I'm done with the first review round.

DmitryVasilevsky · 2021-07-07T18:59:48Z

src/Simulation/Simulators/SparseSimulator/Native/flat_hash_map.hpp

@@ -0,0 +1,1502 @@
+//          Copyright Malte Skarupke 2017.


Is there an STL version of this? Can it be used?

This hash map has better performance than the STL hash map, so it would be better if we can use this one. If we can't, the only functional difference from the STL hash map is the "jump_forward" function defined here, which makes the multi-threading faster.

src/Simulation/Simulators/SparseSimulator/SparseSimulatorTests/TestHelpers.cpp

…ck-2 More fixes from feedback

kuzminrobin · 2021-09-08T23:30:53Z

src/Simulation/Simulators/SparseSimulator/Native/factory.cpp

+    size_t emptySlot = -1;
+    for (auto const& s : _simulators)
+    {
+        if (s == NULL)


Suggested change

if (s == NULL)

if (s == nullptr)

kuzminrobin · 2021-09-08T23:47:27Z

src/Simulation/Simulators/SparseSimulator/Native/gates.h

+    std::vector<logical_qubit_id> controls;
+    operation(OP gate_type_arg, 
+        logical_qubit_id target_arg, 
+        std::vector<logical_qubit_id> controls_arg


Suggested change

std::vector<logical_qubit_id> controls_arg

const std::vector<logical_qubit_id>& controls_arg

src/Simulation/Simulators/SparseSimulator/Native/quantum_state.hpp

kuzminrobin · 2021-09-09T00:17:03Z

src/Simulation/Simulators/SparseSimulator/Native/quantum_state.hpp

+template<size_t num_qubits>
+class QuantumState : public BasicQuantumState


FYI:
See Item 44: Factor parameter-independent code out of templates in Effective C++: 55 Specific Ways to Improve Your Programs, 3rd edition, by Scott Meyers.

kuzminrobin · 2021-09-09T00:29:45Z

src/Simulation/Simulators/SparseSimulator/Native/quantum_state.hpp

+template<size_t num_qubits>
+class QuantumState : public BasicQuantumState


This file (and this class in particular) will definitely benefit from breaking-up into multiple files.
Or at least it would be good to separate the definition of the class from the definition of the member functions:

// Definition of the class: template<size_t num_qubits> class QuantumState : public BasicQuantumState { public: QuantumState(); // The member functions are declared only, but not defined here. . . . private: // Member variables. }; // Definition of the member functions (if it is a template class then in the same .hpp file): template<size_t num_qubits> QuantumState::QuantumState() { . . . }

kuzminrobin · 2021-09-09T00:55:55Z

src/Simulation/Simulators/SparseSimulator/SparseSimulatorCS/SparseSimulator.cs

+
+            if (isLogging)
+            {
+                stackDepth = stackDepth[0..(stackDepth.Length - 1)];


I don't know C# but I have an impression that this line does not change anything (assigns the value to itself, same as stackDepth = stackDepth).

…ck-3 Unit tests, comments and feedback from review

src/Simulation/Simulators/SparseSimulator/Native/capi.cpp

…ck-3 Some build fixes

…ternal keyword

src/Simulation/Simulators/SparseSimulator/Native/types.h

Include and library dirs for OpenMP

Iterator dereference fix

Used std::unordered_map and removed parallel execution

Initial sparse simulator commit

86b3451