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.

Sign up for GitHub

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

Jump to bottom

Refactor thermostats and add unit tests #3438

Merged
merged 12 commits into from
Jan 27, 2020
Merged

Refactor thermostats and add unit tests #3438

merged 12 commits into from
Jan 27, 2020

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Jan 25, 2020 •
edited
Loading

  • Gather thermostat global variables in structs (Remove global variables #2628)
  • Make thermostat functions pure functions to eliminate side effects
  • Add Boost unit tests for thermostats
  • Reduce thermostat code complexity

jngrad added 12 commits January 22, 2020 22:30
@jngrad
Move Langevin Dynamics globals in a struct
3821370
@jngrad
Move Langevin Dynamics to dedicated header file
ee8f47d
@jngrad
Remove redundant prefixes
3e4bb5e
@jngrad
Move NPT Isotropic thermostat globals in a struct
c1984e2
@jngrad
Merge branch 'python' into boost-thermostats
ffca319
@jngrad
Narrow thermostat globals
7a42d82 
Pass thermostat parameter structs as const reference instead of
using globals.
@jngrad
Move prefactor logic to the structs
b1dd0e5
@jngrad
Hadamard specializations
e74518e
@jngrad
Add unit tests for thermostats
39ff4e7
@jngrad
Refactor thermostats
53d253f 
Narrow interfaces, simplify code and fix include statements.
@jngrad
Refactor thermostats noise sigma calculation
dc47afc 
Simplify and centralize the logic to calculate the noise standard
deviation in Brownian/Langevin thermostats. Replace all the inverse
sigmas by regular sigmas in the Brownian prefactors; those were no
longer needed since the Brownian code is now separate from the
Langevin code.
@jngrad
Refactor thermostats using pure functions
be3e2f3 
Remove side effect and hide implementation details of Particles.
Fuse loops. Remove comments in Brownian functions that walked through
the propagator workflow, because that information is now completely
transparent by reading the body of `brownian_dynamics_propagator()`.
@codecov
Copy link

codecov bot commented Jan 25, 2020 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff            @@
##             python   #3438   +/-   ##
========================================
  Coverage          ?     86%           
========================================
  Files             ?     537           
  Lines             ?   25446           
  Branches          ?       0           
========================================
  Hits              ?   22061           
  Misses            ?    3385           
  Partials          ?       0
Impacted Files Coverage Δ
src/utils/tests/matrix_vector_product.cpp 100% <ø> (ø)
...core/electrostatics_magnetostatics/p3m-dipolar.hpp 96% <ø> (ø)
src/core/electrostatics_magnetostatics/p3m.hpp 100% <ø> (ø)
src/core/global.cpp 83% <ø> (ø)
src/utils/tests/interpolation_test.cpp 100% <100%> (ø)
src/utils/include/utils/Vector.hpp 100% <100%> (ø)
src/utils/tests/Vector_test.cpp 100% <100%> (ø)
src/core/integrators/velocity_verlet_npt.cpp 86% <100%> (ø)
...core/electrostatics_magnetostatics/p3m-dipolar.cpp 82% <100%> (ø)
src/core/forces_inline.hpp 85% <100%> (ø)
... and 15 more

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 c1984e2...be3e2f3. Read the comment docs.

@jngrad jngrad force-pushed the boost-thermostats branch from 51e2190 to be3e2f3 Compare January 25, 2020 19:15
@jngrad jngrad changed the title WIP: Refactor thermostats and add unit tests Refactor thermostats and add unit tests Jan 25, 2020
@jngrad jngrad added this to the Espresso 4.2 milestone Jan 25, 2020
@jngrad
Copy link
Member Author

jngrad commented Jan 25, 2020

  • The thermostats require external linkage for Cython, this is achieved in the new file src/script_interface/Globals.hpp
  • The NpT thermostat cannot be unit tested until we switch it to Philox (Switch NPT to Philox #3119). Once this is done, testing will be trivial, and the NpT prefactors could be renamed to something more intuitive.
  • The naming of the thermostat structs (BrownianThermostat, etc.) is not important. We'll have an opportunity to discuss the naming convention in the future when we organize the refactoring of the integrator and thermostat functionality.
  • There shouldn't be too much of an impact on the Stokesian Dynamics PR (WIP: Stokesian dynamics #3241), but it is accumulating merge conflicts from the multiple integrator/thermostat PRs merged recently. Once Stokesian passes CI, I'll help merging the python branch into it.

@KaiSzuttor KaiSzuttor added the automerge Merge with kodiak label Jan 27, 2020
@kodiakhq kodiakhq bot merged commit 6cb813a into espressomd:python Jan 27, 2020
@jngrad jngrad deleted the boost-thermostats branch January 18, 2022 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fweik fweik fweik approved these changes

Assignees
No one assigned
Labels
automerge Merge with kodiak Core Improvement Testcase
Projects
None yet
Milestone
Espresso 4.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@jngrad @fweik @KaiSzuttor