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

Split LB tutorial and create polymers and Langevin simulation tutorial #4052

Merged
merged 4 commits into from
Aug 17, 2021

Conversation

schlaicha
Copy link
Contributor

Fixes #3939

Description of changes:

  • Create new tutorial 'polymers'
  • Move LB tutorials part 2+3 to polymers
  • Update numbering for consistency

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@schlaicha schlaicha changed the title Split LB tutorial and create polymers tutorial Split LB tutorial and create polymers and Langevin simulation tutorial Dec 18, 2020
@schlaicha
Copy link
Contributor Author

Closing for now until the new points in #3939 are addressed.

@jngrad jngrad added this to the Espresso 4.2 milestone Jul 5, 2021
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.

Sorry, I'm giving up on inline comments. The screen reader cannot handle the huge diff.

Polymers.ipynb:

  • The polyemer creation loop should be written without particle id:
previous = None
for pos in positions:
    p=system.aprt.add(pos=pos)
    if previous is not None:
        p.add_bond((dwnw, pewcioua))
    previous = p

or similar.

  • Please briefly explain "rouse regime" and "zimm regime"
  • Why is the integral in the error estimation hand-written rather than using np.trapz or the like?
  • This tutorial relies on VACF, which is missing in the Langevin tutorial
  • The learning goals for the polymer tutorial are probably somewhat overstated. They should be made more specific, e.g., "explain how hydrodynamic interactions qualitatively affect the diffusion of a polymer. Name the three important regimes for polymer diffusion and the corresponding scaling laws."
  • I believe the considered polymer lengths are too short for the Zimm regime. If one leaves it like that, it needs to be stated clearly in the tutorial that this is done for simulation rime reasons.
  • Is updating the correlator every time step actually the best choice considering accuracy vs computaitonal effort?

LB tutorial

  • Here, also, the learning goals need to be made more specific and testable, e.g., "Explain the basic idea of the Lattice-Boltzmann method", "Setting up a Lattice-Boltzmann fluid in Espresso", "Setting up the coupling of MD particles to a Lattice-Boltzmann fluid", "Describe the geomtry and expeted flow field of a Poiseuille flow".
  • Theory: The ratio of performance between gpu and cpu is 10, not 100. It should also be mentioned that the gpu code is single precision.
  • It should be mentioned that the required features are ativated in the defualt configuration (i.e., no custom myconfig.hpp)
  • In the section on per-node properties, mention that slicing is supported

Poiseuille flow:

  • the data gathering should use slices rather than looping over coordinates manually

doc/tutorials/langevin_dynamics/NotesForTutor.md Outdated Show resolved Hide resolved
doc/tutorials/langevin_dynamics/langevin_dynamics.ipynb Outdated Show resolved Hide resolved
doc/tutorials/langevin_dynamics/langevin_dynamics.ipynb Outdated Show resolved Hide resolved
doc/tutorials/langevin_dynamics/langevin_dynamics.ipynb Outdated Show resolved Hide resolved
doc/tutorials/langevin_dynamics/langevin_dynamics.ipynb Outdated Show resolved Hide resolved
doc/tutorials/langevin_dynamics/langevin_dynamics.ipynb Outdated Show resolved Hide resolved
kodiakhq bot added a commit that referenced this pull request Aug 13, 2021
Part of #4052 

Description of changes:
- Add Green-Kubo calculation of the diffusivity. To get at least somewhat acceptably smooth VACFs, the number of simulation steps was increased from 400000 to 1000000, the simulation time is still quite acceptable, I think.
- General improvements, including changes requested by @RudolfWeeber and @christophlohrmann 
- Proper additon of hidden solutions
@jngrad jngrad self-requested a review August 16, 2021 16:36
jngrad and others added 3 commits August 17, 2021 12:59
Co-authored-by: Rudolf Weeber <weeber@icp.uni-stuttgart.de>
Co-authored-by: Christoph Lohrmann <clohrmann@icp.uni-stuttgart.de>
@jngrad jngrad dismissed RudolfWeeber’s stale review August 17, 2021 13:33

Changes have been applied.

Copy link
Member

@jngrad jngrad left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks to all contributors!

@jngrad jngrad added automerge Merge with kodiak Documentation labels Aug 17, 2021
@kodiakhq kodiakhq bot merged commit 1b6d6a3 into espressomd:python Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split LB tutorial into LB, polymer and Langevin simulation tutorials
5 participants