spin_op performance enhancement

[amccaskey]

Update spin_op to use a unordered_map<vector<bool>, complex<double>> as the primary container for pauli terms.

Below are results for a large (15 qubit) random spin_op addition + multiplication operations over a range of term numbers.

[boschmitt]

I added a few preliminary comments.

Could you provide a comparison between this enhancement and the current implementation?
Do you know why changing using std::unordered_map provides better performance ?

spin_op's public API currently relies (or leaks) the fact that its current underlying implementation behaves like a vector, e.g., operator[] (size_t index) and spin_op::slice. If we use a std::unordered_map as the underlying data structure to store terms, these methods don't make much sense and will likely cause various bugs.

[anthony-santana]

The code and performance improvements look great! Most comments are related to either nits in the tests or replacing certain member functions with properties.

[amccaskey]

I added a few preliminary comments.

Could you provide a comparison between this enhancement and the current implementation? Do you know why changing using std::unordered_map provides better performance ?

spin_op's public API currently relies (or leaks) the fact that its current underlying implementation behaves like a vector, e.g., operator[] (size_t index) and spin_op::slice. If we use a std::unordered_map as the underlying data structure to store terms, these methods don't make much sense and will likely cause various bugs.

@boschmitt

Great points to bring up.

The current implementation was never tested for performance / speed as the number of terms / qubits grew (everything was fine for the small applications we'd run up to this point). My early tests show that the same benchmark as above is a factor of 100 slower than this unordered_map implementation (hence the need for this PR 😄 ).

The benefits of the unordered_map come in with regards to storing unique Pauli terms, where the uniqueness is enforced via the map key / hash, and like terms are updated by just updating the value (the term coefficient). This becomes more important as we start multiplying spin_op instances with multiple terms (P0 + P1 + ...) * (Q0 + Q1 + ...), which results in many multiplication and addition operations to update the underlying spin_op data structure (vector of vectors in the current implementation vs an unordered_map in this PR). To perform this operation, one has to find like terms (vector<bool>) and compare against others newly created by the multiplication / addition operations. std::vector search complexity is O(N) while unordered_map should be O(1) (granted I think this ignores constant factors, but I think the proof is in the benchmark above). Moreover, I found it easier to reason about multi-threaded multiplications with the unordered_map at the cost of extra memory. Hence the addition of OpenMP parallelization for operator*=().

As for this public API, you are correct that this changes how downstream programmers can / should reason about the underlying terms. I should remove the operator[] method and replace with begin() / end() methods returning iterators that allow range-based for loops with structured bindings. The documentation on the class should also be updated to tell the programmer that any underlying order of the terms should not be relied on as the order is no non-deterministic. I think that slice or something like it can still exist, since there are N key/values in the map and the use case for slice is primarily to extract equally sized chunks for parallel processing. Maybe the name needs to change?

Open to more comments / feedback here in an effort to make this even better. Maybe I've missed something?

[amccaskey]

@boschmitt Latest commit removes slice and replaces with distribute_terms. All of your comments should be addressed now.

[schweitzpgi]

LGTM