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

Documentation of VV, NPT, Thermostats #2939

Merged
merged 9 commits into from
Jul 25, 2019

Conversation

christophlohrmann
Copy link
Contributor

Fixes #2680

Description of changes:

  • Added sphinx documentation for velocity verlet, NPT
  • Added/Updated sphinx documentation for Langevin, NPT, LB, DPD thermostat
  • Found some papers as reference

PR Checklist

  • Tests?
    • Interface
    • Core
  • Docs?

with a single position ``pos`` as an argument can be used.
For the GPU fluid :class:`espressomd.lb.LBFluidGPU`
also :py:attr:`espressomd.lb.LBFluidGPU.get_interpolated_fluid_velocity_at_positions()`
is available, which expects a numpy array of positions as an argument.
Copy link
Member

Choose a reason for hiding this comment

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

could you please also mention the issue of interpolating velocities near boundaries?

@codecov
Copy link

codecov bot commented Jun 25, 2019

Codecov Report

Merging #2939 into python will decrease coverage by <1%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           python   #2939    +/-   ##
=======================================
- Coverage      83%     82%    -1%     
=======================================
  Files         520     519     -1     
  Lines       26746   26958   +212     
=======================================
- Hits        22328   22312    -16     
- Misses       4418    4646   +228
Impacted Files Coverage Δ
src/core/electrostatics_magnetostatics/mmm1d.cpp 31% <0%> (-64%) ⬇️
src/core/nonbonded_interactions/thole.hpp 75% <0%> (-25%) ⬇️
src/core/specfunc.cpp 58% <0%> (-24%) ⬇️
...e/electrostatics_magnetostatics/coulomb_inline.hpp 89% <0%> (-6%) ⬇️
src/core/virtual_sites/VirtualSitesRelative.cpp 81% <0%> (-4%) ⬇️
src/core/electrostatics_magnetostatics/coulomb.cpp 78% <0%> (-1%) ⬇️
src/core/communication.cpp 94% <0%> (-1%) ⬇️
src/core/electrostatics_magnetostatics/p3m.cpp 85% <0%> (-1%) ⬇️
src/core/grid_based_algorithms/lb_interface.cpp 74% <0%> (-1%) ⬇️
src/core/cells.cpp 90% <0%> (-1%) ⬇️
... and 39 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 ad319c0...47494fe. Read the comment docs.


The keyword ``seed`` controls the state of the random number generator (Philox
Counter-based RNG) and is required on first activation of the thermostat. It
can be omitted in subsequent calls of ``set_langevin()``. It is the user's
responsibility to decide whether the thermostat should be deterministic (by
using a fixed seed) or not (by using a randomized seed).

The diffusion coeffictient :math:`D` of the particle can be obtained by the Einstein-Smoluchowski relation
Copy link
Member

Choose a reason for hiding this comment

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

We should discuss to what degree the plain Einstein-Smoluchowski relation is applicable to the gamma which is set in the thermostat. In my mind what is presented in the text only holds for ideal systems. Interactions in the system will modify the DIffusion coefficient -- otherwise there would never be a need to measure diffusion coefficients using the mean squared displacement or a green kubo formula in MD simulations.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @jonaslandsgesell, the documentation should not make statements about the diffusion coefficient here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonaslandsgesell I would remove the mention of the diffusion coefficient entirely, as @fweik suggested.

Copy link
Member

Choose a reason for hiding this comment

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

Hey Christoph. Why not removing it?yes, I mean for ideal (noninteracting ) particles one should observe D=kT/gamma_thermostat. So we state that gamma is the friction of the thermostat somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We say that gamma is the bare friction coefficient for the Langevin equation and any physics beyond that should be the user's responsibility.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

@christophlohrmann
Copy link
Contributor Author

Concerning the dimensions enabled/disabled for scaling in the NPT algorithm:

The corresponding code has not been touched since the original commit from 2004.
The author (Ira Cooke) did not describe this method in any of his papers and I cannot
find his dissertation on the Max Planck homepage. Also I did not find any other paper
using that method.

Should I just mark that option as not tested and leave it like that?

@KaiSzuttor KaiSzuttor added this to the Espresso 4.1 milestone Jul 15, 2019
@jngrad
Copy link
Member

jngrad commented Jul 16, 2019

mark NPT as experimental and update "Usage Status = None" in:
https://github.com/espressomd/espresso/blob/python/doc/sphinx/introduction.rst#available-simulation-methods

Copy link
Contributor

@RudolfWeeber RudolfWeeber left a comment

Choose a reason for hiding this comment

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

bors r+

@jngrad
Copy link
Member

jngrad commented Jul 25, 2019

bors r-

@bors
Copy link
Contributor

bors bot commented Jul 25, 2019

Canceled

@RudolfWeeber
Copy link
Contributor

I checked NPT against Monte Carlo volume moves. It seems to work, in principle.

The PR improves the docs alot. I'd suggest not to defer much longer and instead open a new pr if there is something to add.

@jngrad
Copy link
Member

jngrad commented Jul 25, 2019

bors r=RudolfWeeber

bors bot added a commit that referenced this pull request Jul 25, 2019
2939: Documentation of VV, NPT, Thermostats r=RudolfWeeber a=christophlohrmann

Fixes #2680 

Description of changes:
 - Added sphinx documentation for velocity verlet, NPT
- Added/Updated sphinx documentation for Langevin, NPT, LB, DPD thermostat
- Found some papers as reference


PR Checklist
------------
 - [ ] Tests?
   - [ ] Interface
   - [ ] Core 
 - [x] Docs?


3006: Reduce usage of local cells r=fweik a=KaiSzuttor

Fixes part of #2899

Description of changes:
 - replacement for #2991 

3008: Removed from __future__ r=jngrad a=jrfinkbeiner

Fixes #3004

3015: Update checkpointing docs r=jngrad a=RudolfWeeber

Fixes #2808

Co-authored-by: Christoph Lohrmann <clohrmann@icp.uni-stuttgart.de>
Co-authored-by: Jean-Noël Grad <jgrad@icp.uni-stuttgart.de>
Co-authored-by: Kai Szuttor <kai@icp.uni-stuttgart.de>
Co-authored-by: Alexander Reinauer <st144434@stud.uni-stuttgart.de>
Co-authored-by: Jan Finkbeiner <st144298@stud.uni-stuttgart.de>
Co-authored-by: Rudolf Weeber <weeber@icp.uni-stuttgart.de>
@bors
Copy link
Contributor

bors bot commented Jul 25, 2019

Timed out (retrying...)

1 similar comment
@bors
Copy link
Contributor

bors bot commented Jul 25, 2019

Timed out (retrying...)

bors bot added a commit that referenced this pull request Jul 25, 2019
2939: Documentation of VV, NPT, Thermostats r=RudolfWeeber a=christophlohrmann

Fixes #2680 

Description of changes:
 - Added sphinx documentation for velocity verlet, NPT
- Added/Updated sphinx documentation for Langevin, NPT, LB, DPD thermostat
- Found some papers as reference


PR Checklist
------------
 - [ ] Tests?
   - [ ] Interface
   - [ ] Core 
 - [x] Docs?


3006: Reduce usage of local cells r=fweik a=KaiSzuttor

Fixes part of #2899

Description of changes:
 - replacement for #2991 

Co-authored-by: Christoph Lohrmann <clohrmann@icp.uni-stuttgart.de>
Co-authored-by: Jean-Noël Grad <jgrad@icp.uni-stuttgart.de>
Co-authored-by: Kai Szuttor <kai@icp.uni-stuttgart.de>
Co-authored-by: Alexander Reinauer <st144434@stud.uni-stuttgart.de>
@bors
Copy link
Contributor

bors bot commented Jul 25, 2019

Build succeeded

@bors bors bot merged commit e64e5f7 into espressomd:python Jul 25, 2019
@bors
Copy link
Contributor

bors bot commented Jul 25, 2019

Merge conflict (retrying...)

@bors
Copy link
Contributor

bors bot commented Jul 25, 2019

Merge conflict

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.

Integration scheme not documented
6 participants