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

Tutorial 1 - Lennard-Jones #3881

Merged
merged 49 commits into from
Sep 30, 2020
Merged

Conversation

schlaicha
Copy link
Contributor

Refactor and improve the LJ tutorial.

Fixes #3818

Description of changes:

  • now use to zero-based indexing in the loop

now use to zero-based indexing in the loop, see espressomd#3818
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

Review Jupyter notebook visual diffs & provide feedback on notebooks.


Powered by ReviewNB

@RudolfWeeber
Copy link
Contributor

We will not be able to support alternative solutions. It would require running the test several times.

The one without the loop is better practice (more declarative), so let's keep that.

@RudolfWeeber
Copy link
Contributor

I added learning objectives to the tutor's notes

@schlaicha
Copy link
Contributor Author

We will not be able to support alternative solutions. It would require running the test several times.

The one without the loop is better practice (more declarative), so let's keep that.

Agreed and pushed in 7562149

@schlaicha
Copy link
Contributor Author

@jngrad I accidentally deleted my comments in the started review discussion. Only thing left from there is a possibility to check all the links in the notebooks using the CMake infrastructure.

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.

Thanks for writing links to the user guide, this will be very helpful to new users!

Other points to address, that are not part of the diff:

  • The MSD correlator explanation is no longer accurate. The first two columns don't store the lag time and sample size anymore, instead one has to call msd_corr.lag_times() explicitly to get the lag times. This regression was introduced in a previous PR.
  • The introduction paragraph needs some cleanup. The core is no longer written in the C language.

doc/tutorials/01-lennard_jones/01-lennard_jones.ipynb Outdated Show resolved Hide resolved
doc/tutorials/01-lennard_jones/01-lennard_jones.ipynb Outdated Show resolved Hide resolved
doc/tutorials/01-lennard_jones/01-lennard_jones.ipynb Outdated Show resolved Hide resolved
doc/tutorials/01-lennard_jones/NotesForTutor.md Outdated Show resolved Hide resolved
doc/tutorials/01-lennard_jones/01-lennard_jones.ipynb Outdated Show resolved Hide resolved
schlaicha and others added 4 commits September 4, 2020 14:10
Co-authored-by: Jean-Noël Grad <[email protected]>
Co-authored-by: Jean-Noël Grad <[email protected]>
Co-authored-by: Jean-Noël Grad <[email protected]>
Co-authored-by: Jean-Noël Grad <[email protected]>
@jngrad
Copy link
Member

jngrad commented Sep 4, 2020

@schlaicha please use batch commits, we have limited CI resources :)

@schlaicha
Copy link
Contributor Author

Sorry, was too nice to just click on the commit change button. Is there a way to batch commit on the github UI or do I need to manually do this?

@jngrad
Copy link
Member

jngrad commented Sep 4, 2020

click "Add suggestion to batch" (GitHub docs)

This also requires to shift the initialization of the thermostat to
after the steepest descent.
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.

For the most part, minor fixes. If the overlap removal would become somewhat simpler, that would be great for a beginner's tutorial.

doc/tutorials/01-lennard_jones/01-lennard_jones.ipynb Outdated Show resolved Hide resolved
doc/tutorials/01-lennard_jones/01-lennard_jones.ipynb Outdated Show resolved Hide resolved
doc/tutorials/01-lennard_jones/01-lennard_jones.ipynb Outdated Show resolved Hide resolved
doc/tutorials/01-lennard_jones/01-lennard_jones.ipynb Outdated Show resolved Hide resolved
doc/tutorials/01-lennard_jones/01-lennard_jones.ipynb Outdated Show resolved Hide resolved
doc/tutorials/01-lennard_jones/01-lennard_jones.ipynb Outdated Show resolved Hide resolved
@RudolfWeeber
Copy link
Contributor

RudolfWeeber commented Sep 27, 2020 via email

@schlaicha
Copy link
Contributor Author

I’m confused. The steepest descent code in espresso (to my knowledge) is force based. It moves particles by a certain amount along the force vectors acting on them.

I'm sorry, of course you're right. On a Sunday evening I missed that grad x E is equal to the force. And indeed my argumentation is exactly opposite for MD/MC.

Isn’t that exactly what’s needed?

Yes, only that in general I would not know what value fmax is good enough for a specific system / timestep. My suggestion was to have a relative convergence criterion directly in (which would simplify the typical simulation script/tutorial). But maybe that is also not needed.

Could you please make an attempt to simplify the loop? A single criterion such as absolute force should be enough, IMO.

So removing the energy convergence criterion is simple enough? I would not see how to make it much simpler otherwise...

@RudolfWeeber
Copy link
Contributor

RudolfWeeber commented Sep 28, 2020 via email

RudolfWeeber
RudolfWeeber previously approved these changes Sep 29, 2020
@RudolfWeeber RudolfWeeber added the automerge Merge with kodiak label Sep 29, 2020
doc/tutorials/01-lennard_jones/NotesForTutor.md Outdated Show resolved Hide resolved
doc/tutorials/01-lennard_jones/NotesForTutor.md Outdated Show resolved Hide resolved
doc/tutorials/01-lennard_jones/NotesForTutor.md Outdated Show resolved Hide resolved
doc/tutorials/01-lennard_jones/NotesForTutor.md Outdated Show resolved Hide resolved
doc/tutorials/01-lennard_jones/01-lennard_jones.ipynb Outdated Show resolved Hide resolved
doc/tutorials/01-lennard_jones/01-lennard_jones.ipynb Outdated Show resolved Hide resolved
doc/tutorials/01-lennard_jones/01-lennard_jones.ipynb Outdated Show resolved Hide resolved
doc/tutorials/01-lennard_jones/01-lennard_jones.ipynb Outdated Show resolved Hide resolved
schlaicha and others added 8 commits September 30, 2020 09:25
Triggers an error message if the exact same python interpreter build
cannot be found on the computer, with a message prompt to select one
of the available python interpreters.
"## Comparison with the literature\n",
"\n",
"Empirical radial distribution functions have been determined for pure fluids <a href='#[5]'>[5]</a>, mixtures <a href='#[6]'>[6]</a> and confined fluids <a href='#[7]'>[7]</a>. We will compare our distribution $g(r)$ to the theoretical distribution $g(r^*, \\rho^*, T^*)$ of a pure fluid <a href='#[5]'>[5]</a>."
"From the roughly exponential decay of the correlation function it dawns that only every second sample should be used to calculate statistically independent averages."
Copy link
Member

Choose a reason for hiding this comment

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

My lecture on error estimation advises against this practice because it can lead to an overestimation of the standard error of the mean. I think it is better practice to carry out binning analysis, or better, autocorrelation function integration.

@kodiakhq kodiakhq bot merged commit d59a47c into espressomd:python Sep 30, 2020
@jngrad jngrad added this to the Espresso 4.2 milestone Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge with kodiak
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tutorial 1: LJ
4 participants