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

Remove ESPResSo system seed mechanism #3482

Merged
merged 7 commits into from
Feb 14, 2020

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Feb 13, 2020

All thermostats are now Philox-based. The Mersenne Twister system seed infrastructure (global variables, MPI callbacks, Python bindings) is no longer needed.

Remove assignments to System.random_number_generator_state and
System.seed. Remove calls to System.set_random_state_PRNG().
Discard the first 1'000'000 values of the RNG to escape Zeroland
(see Figure 4 in Panneton et al. 2006).
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB.

@codecov
Copy link

codecov bot commented Feb 13, 2020

Codecov Report

❗ No coverage uploaded for pull request base (python@915df65). Click here to learn what that means.
The diff coverage is 92%.

Impacted file tree graph

@@           Coverage Diff            @@
##             python   #3482   +/-   ##
========================================
  Coverage          ?     87%           
========================================
  Files             ?     536           
  Lines             ?   24400           
  Branches          ?       0           
========================================
  Hits              ?   21247           
  Misses            ?    3153           
  Partials          ?       0
Impacted Files Coverage Δ
src/core/polymer.cpp 95% <92%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 915df65...29266d1. Read the comment docs.

@jngrad
Copy link
Member Author

jngrad commented Feb 14, 2020

espressomd.polymer.linear_polymer_positions has become flaky. I can reproduce the error with ctest --repeat-until-fail 1000 --output-on-failure -R polymer_linear (edit: with seed = random.randint(0,1000) to change the seed, otherwise the test always passes). If I print the Mersenne Twister seed from a failed test and re-use it in the test script, the test fails every time. The only change to polymer.cpp is that its Mersenne Twister now discards the first 1 million values to remove correlation.

auto rng = [mt = std::mt19937{static_cast<unsigned>(seed)},
dist = std::uniform_real_distribution<double>(
0.0, 1.0)]() mutable { return dist(mt); };
auto mt = Random::mt19937(static_cast<unsigned>(seed));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why you widened the scope of mt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch, I originally moved mt out to discard the first 1 million values, but then I made a wrapper in random.hpp to avoid the situation where MT would used without warmup. I'll put it back in the capturing group.

@fweik
Copy link
Contributor

fweik commented Feb 14, 2020

The polymer generation can fail, so it should not have a random seed. Why would you expect this to work?

@jngrad
Copy link
Member Author

jngrad commented Feb 14, 2020

Why would the fixed seed in the test script generate correct positions in all machines except osx? If the MT rng produces random values that are architecture-dependent, fixing the seed is useless.

@fweik
Copy link
Contributor

fweik commented Feb 14, 2020

If the MT rng produces random values that are architecture-dependent

is that so?

@fweik
Copy link
Contributor

fweik commented Feb 14, 2020

The 10000th consecutive invocation of a default-constructed std::mt19937 is required to produce the value 4123659995.

The 10000th consecutive invocation of a default-constructed std::mt19937_64 is required to produce the value 9981545732273789042

@jngrad
Copy link
Member Author

jngrad commented Feb 14, 2020

I wasn't sure, that's why I used "if". I'm looking through polymer.cpp to see if something is not deterministic. The OpenGL visualizer is also currently broken on my machine, so I can't visually check the algorithm output.

@jngrad jngrad added the automerge Merge with kodiak label Feb 14, 2020
@kodiakhq kodiakhq bot merged commit 4612591 into espressomd:python Feb 14, 2020
@jngrad jngrad deleted the remove-mersenne-twister branch January 18, 2022 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants