Skip to content
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

Fixed bug in parallelization in MPS simulator. #292

Merged
merged 8 commits into from
Jul 24, 2019

Conversation

merav-aharoni
Copy link
Contributor

@merav-aharoni merav-aharoni commented Jul 22, 2019

Summary

There was an "omp parallel" on a for loop that had a vector that was assigned values using push_back. That caused undeterministic behavior. Changed the push_back to assignment.

Details and comments

@merav-aharoni merav-aharoni changed the title Fixed bug in parallelization. It does not work correctly with vector … Fixed bug in parallelization in MPS simulator. Jul 22, 2019
Copy link
Member

@atilag atilag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!
Standard containers are no thread-safe for writing (they are for reading).

Copy link
Member

@atilag atilag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the later rush review. I'm a bit concern about the fact that our tests didn't catch this issue. Can you think of a test that would catch similar problems in the future?
Thanks!

#pragma omp parallel for
#else
#pragma omp parallel for collapse(2)
#endif
for(uint_t row = 0; row < num_rows; row++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to change the type of row to int_t due to incompatibilities with Windows OpenMP support.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected both places as you pointed out.
It is a problem to catch failures that only happen non-deterministically. I can only suggest running the unittest a few times.
We also use another kind of testing here - we create random circuits and then compare results between the mps and the statevector. If you are interested, I can add these somewhere as well.

#pragma omp parallel for
#else
#pragma omp parallel for collapse(2)
#endif
for(uint_t row = 0; row < num_rows; row++) {
for(uint_t col = 0; col < num_cols; col++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to chage this type too: uint_t col => int_t col

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above.

atilag
atilag previously approved these changes Jul 24, 2019
Copy link
Member

@atilag atilag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this is good to go.
Just need to add the PR number to the end of the CHANGELOG entry and I'll merge :)

CHANGELOG.md Outdated
@@ -35,6 +35,7 @@ Removed

Fixed
-----
- Bug in handling parallelization in matrix_product_state.cpp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add the number of the PR at the end of this line (see the other entries) :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right. done.

@atilag atilag merged commit 92607b7 into Qiskit:master Jul 24, 2019
@merav-aharoni merav-aharoni deleted the bug-fix branch July 24, 2019 13:03
hhorii pushed a commit to hhorii/qiskit-aer that referenced this pull request Jul 25, 2019
* Fixed bug in parallelization. It does not work correctly with vector push_back. Need to use assignment instead.
dcmckayibm pushed a commit to dcmckayibm/qiskit-aer that referenced this pull request Nov 3, 2019
* Fixed bug in parallelization. It does not work correctly with vector push_back. Need to use assignment instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants