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

It seems that the Dense(M, N, min, max) constructor is not completely random. #70

Closed
lotz84 opened this issue Sep 12, 2021 · 5 comments
Closed

Comments

@lotz84
Copy link

lotz84 commented Sep 12, 2021

Running the following simple program

#include <iostream>
#include <monolish_blas.hpp>

int main() {
  monolish::matrix::Dense<double>x(2, 3, 0.0, 10.0);
  x.print_all();
  return 0;
}
$ g++ -O3 main.cpp -o main.out -lmonolish_cpu

will produce results like this.

root@636020fa2fd8:/$ ./main.out
1 1 5.27196
1 2 2.82358 <--
1 3 2.13893 <--
2 1 9.72054
2 2 2.82358 <--
2 3 2.13893 <--
root@636020fa2fd8:/$ ./main.out
1 1 5.3061
1 2 9.75236
1 3 7.15652
2 1 5.28961
2 2 2.05967
2 3 0.59838
root@636020fa2fd8:/$ ./main.out
1 1 9.33149 <--
1 2 4.75639 <--
1 3 8.71093 <--
2 1 9.33149 <--
2 2 4.75639 <--
2 3 8.71093 <--

The arrows (<--) indicate that the number is repeating.

This is probably due to that the pseudo-random number generator does not split well when it is parallelized by OpenMP.

std::random_device random;
std::mt19937 mt(random());
std::uniform_real_distribution<> rand(min, max);
#pragma omp parallel for
for (size_t i = 0; i < val.size(); i++) {
val[i] = rand(mt);
}

This may happen not only with Dense, but also with random constructors of other data structures.

I tested this on docker image ghcr.io/ricosjp/monolish/mkl:0.14.1.

@t-hishinuma
Copy link
Contributor

Thanks for reporting the bug.
You're probably right. I also think it's a problem with OpenMP parallelization.

I'll check and fix it next week.

@t-hishinuma
Copy link
Contributor

This is due to the fact that random_device is declared thread-local.
This bug occurs in the following two functions:

On the other hand, the random_vector function in include/common/monolish_common.hpp is not parallelized.

I will parallelize all functions, declaring the random_vector variable as a local variable of the thread.

@t-hishinuma
Copy link
Contributor

I used the following test program to test it. This is not included in test/.

#include<monolish_blas.hpp>

int main(){
    monolish::vector<double> x(100000, 0.1, 1.0);
    x.print_all();

    return 0;
}

./a.out | uniq | wc

@t-hishinuma
Copy link
Contributor

I have fixed the bug with the following commit.
3ccf1d6

Thanks for your reports!

@lotz84
Copy link
Author

lotz84 commented Sep 14, 2021

Quick and appropriate fix! Thank you.

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

No branches or pull requests

2 participants