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

Random123 behavior tests #2773

Merged
merged 55 commits into from
Apr 23, 2024
Merged

Random123 behavior tests #2773

merged 55 commits into from
Apr 23, 2024

Conversation

atemerev
Copy link
Contributor

Included a smoke test to see if Random123 actually works (it is fully deterministic, so we should get exactly the same output for a particular set of parameters).

Probably should also include some functional tests to see if our distribution generators work too.

Copy link

✔️ ff25ae7 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Copy link

✔️ e3befee -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Copy link

codecov bot commented Mar 12, 2024

Codecov Report

Attention: Patch coverage is 95.23810% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 67.20%. Comparing base (8383628) to head (ea1930c).

Files Patch % Lines
test/coreneuron/unit/random/test_random.cpp 95.23% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2773      +/-   ##
==========================================
- Coverage   67.20%   67.20%   -0.01%     
==========================================
  Files         564      565       +1     
  Lines      104246   104277      +31     
==========================================
+ Hits        70060    70080      +20     
- Misses      34186    34197      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@pramodk pramodk left a comment

Choose a reason for hiding this comment

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

Nice!

I have few minor comments that we can address and we get this merged.

src/nrnoc/hh.mod Outdated Show resolved Hide resolved
test/coreneuron/unit/random/test_random.cpp Outdated Show resolved Hide resolved
Copy link

✔️ 2a588a2 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@atemerev atemerev marked this pull request as draft April 10, 2024 12:02
@atemerev
Copy link
Contributor Author

The test is not working with OpenACC/OpenMP, because it is exactly the failure mode of stateful RNGs in the parallel environment :) I can rewrite it properly, but the question is what exactly we should test, there are many options (e.g. there should be no duplicate values etc).

@nrnhines
Copy link
Member

because it is exactly the failure mode of stateful RNGs in the parallel environment

The usage is always a separate stream for every physical element that needs a random stream. All those physical elements are potentially computed in parallel. So I think it is most realistic for your test to have a double loop where the outer loop represents time steps and the inner loop is over elements. i.e. something like

for (int istep = 0; i < nstep; ++istep) {
    nrn_pragma_acc(parallel loop copy(res [0:nstream]) deviceptr(s))
    for (int istream = 0; i < nstream; ++istream) {
        res[istep][istream] = nrnran123_dblpick(s[istream]);
    }
}

Every res element is statistically independent of all the other res elements in the interval 0 to 1 (I'm not sure 0.0 or 1.0 are part of that interval.) And res should be the same regardless of the degree of omp or acc parallelization.

Copy link

✔️ ffe0d2b -> Azure artifacts URL

Copy link

✔️ acd61d8 -> Azure artifacts URL

@atemerev atemerev marked this pull request as ready for review April 18, 2024 13:05
@atemerev atemerev requested a review from pramodk April 18, 2024 13:05
Copy link

✔️ 0e19aa1 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Copy link

✔️ fd9bb33 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@atemerev atemerev requested a review from alkino April 22, 2024 07:56
Copy link

✔️ 69d9d5b -> Azure artifacts URL

Copy link

sonarcloud bot commented Apr 23, 2024

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

✔️ ea1930c -> Azure artifacts URL

@alkino alkino merged commit 1adc8f5 into master Apr 23, 2024
37 checks passed
@alkino alkino deleted the atemerev/random123-test branch April 23, 2024 09:18
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.

5 participants