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

Refactor thermostats: RNG, callbacks and documentation #3444

Merged
merged 8 commits into from
Feb 4, 2020

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Feb 3, 2020

Description of changes:

  • switch NPT thermostat RNG from Mersenne twister to Philox (fixes Switch NPT to Philox #3119)
  • add extra RNG correlation checks in unit tests using boost accumulators
  • make thermostats object-oriented and generate their MPI callbacks with macros

Check the mean and variance of random noise, and the absence of
correlation. Check the absence of correlation between the 3 axes
of positions and velocities generated by the thermostats.
Handle the case of PRNGs generating vectors of size 1 to 4. Check
for correlation across both axes and quantities (e.g. correlation
between translational velocity and rotational velocity). Also test
the boost::variant infrastructure.
Use struct inheritance for thermostats. Generate thermostat MPI
callbacks using a macro.
API change: system.thermostat.set_npt() now requires a seed.
The Mersenne twister was replaced by the Philox generator. The
compressibility has changed from 0.20 to 0.32. This new value
is reproducible within delta=0.01 after 20'000 integration steps
for different random seeds.
@@ -94,8 +91,13 @@ extern bool thermo_virtual;
* parameter structs
************************************************/

struct BaseThermostat {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain why the base class is needed? As far as I can tell the inheritance is not used, and in general (mutable) data members in base classes are a bad idea (due to the non-locality of mutation).

Copy link
Member Author

Choose a reason for hiding this comment

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

I originally thought about moving most of the thermostat RNG MPI callbacks into the base struct as getters and setters, and only have a few MPI callbacks (not macro-generated) taking as argument the thermostat switch to select from which global object to call the setter/getter, but these plans didn't materialize. I wouldn't mind reverting that inheritance; it's probably too early to look into that yet.

If by non-locality of mutation you mean the ability of Brownian to change the value of the Langevin counter, I'm not sure how we could run into that situation, as having a BaseThermostat method that takes a ref to another BaseThermostat object is not useful.

@codecov
Copy link

codecov bot commented Feb 3, 2020

Codecov Report

Merging #3444 into python will increase coverage by <1%.
The diff coverage is 93%.

Impacted file tree graph

@@           Coverage Diff           @@
##           python   #3444    +/-   ##
=======================================
+ Coverage      86%     86%   +<1%     
=======================================
  Files         539     539            
  Lines       25658   25685    +27     
=======================================
+ Hits        22264   22290    +26     
- Misses       3394    3395     +1
Impacted Files Coverage Δ
src/utils/tests/tuple_test.cpp 100% <100%> (ø) ⬆️
src/core/bonded_interactions/bonded_tab.cpp 91% <100%> (ø) ⬆️
...re/bonded_interactions/bonded_interaction_data.hpp 100% <100%> (ø) ⬆️
src/utils/include/utils/tuple.hpp 100% <100%> (ø) ⬆️
...re/bonded_interactions/bonded_interaction_data.cpp 91% <86%> (-9%) ⬇️
src/core/electrostatics_magnetostatics/p3m.cpp 85% <0%> (ø) ⬆️

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 7f7dda5...4047fdb. Read the comment docs.

@fweik fweik self-assigned this Feb 4, 2020
@fweik fweik added the automerge Merge with kodiak label Feb 4, 2020
@kodiakhq kodiakhq bot merged commit 8e9d094 into espressomd:python Feb 4, 2020
@jngrad jngrad deleted the fix-3119 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.

Switch NPT to Philox
2 participants