-
Notifications
You must be signed in to change notification settings - Fork 197
MLIP and Water Tutorial #5174
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
base: python
Are you sure you want to change the base?
MLIP and Water Tutorial #5174
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Thank you! It looks very good. A few points from an initial quick read: Part 1
Part 2
Part 3
|
| @@ -0,0 +1,583 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it not be more readable/less error prone to get the constants from scipy.constants?
And if you use pint and units everywhere, also the lengths should get there units IMHO (and for the charges and masses below). Alternatively (maybe preferred), add the explanation that these constants are defined in order to convert to the unit-less quantities below - as it is now it is not clear intuitively which quantities have or don't have units.
Reply via ReviewNB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified this cell for using Scipy.constants and added "these constants are defined in order to convert to the unit-less quantities below" below the cell.
| @@ -0,0 +1,583 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line #4. def rigid_constrain(p, C):
Why C? This relates two particle positions, right? Maybe center would be more instructive?
Reply via ReviewNB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified the argument C to center_mol becuase center is already defined.
| @@ -0,0 +1,583 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typiically the hydrogens have no LJ interaction, but for example in the CHARMM compatible versions they do. To be fair I would simply add the typical in the sentence.
Reply via ReviewNB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added typical in front of TIP4P.
| @@ -0,0 +1,583 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line #2. epsilon=OO_EPSILON, sigma=OO_SIGMA, cutoff=0.85, shift="auto"
This is the original cutoff, and water properties at this small cutoff are very sensitive
Reply via ReviewNB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified cutoff from 2.5 * OO_SIGMA to 0.85.
| @@ -0,0 +1,583 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line #3. system.thermostat.set_langevin(kT=1, gamma=1, seed=1)
This will not give the correct dynamics. I see that we have no other choices for the thermostat, but at least we should tune the friction coefficient to match the dynamics of TIP4P/2005,
Would this yield the desired behavior?
Reply via ReviewNB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this tutorial, we only discuss the radial distribution function. Then, for fast equilibrium, we set gamma=1.
| @@ -0,0 +1,583 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it.
| @@ -0,0 +1,583 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line #5. r_max = 0.7 # nm
Why not L/2? Then this looks much more familar...
Correspondingly
rdf_bins = int(L/2 * 100)
Reply via ReviewNB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this tutorial, all RDFs are compared with each other. For the third part, the maximum length for RDF is 7 angstroms since the system size is 14 angstroms. To match that length, we set it to 7 angstroms.
| @@ -0,0 +1,583 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line #5. data = np.load("rdf_experiment.npz")
File is missing, so I cannot judge on the quality of the rdf...
Reply via ReviewNB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After using $dvc pull, the necessary data is provided.
|
I now had a look at the first part, this is looking very impressive to me (see my comments above)! I just did a slightly longer (100x) run to test the pressure in the simulation, at least with the current friction coefficient (see my comment above) it seems reasonable: I will look into parts 2 and 3 tomorrow. |
Fixes espressomd#5171, fixes espressomd#5035 - Remove unused/unnecessary code - Fix zndraw interface not respecting PBC - Introduce bonds drawing - Upgrade zndraw to v0.5.11
Add a single-step symplectic Euler integrator to propagate particles with time-dependent external forces. Co-authored-by: Hideki Kobayashi <[email protected]>
Description of changes: - remove communication overhead in ParticleSlice setters Co-authored-by: Jean-Noël Grad <[email protected]>
| @@ -0,0 +1,742 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe one should add a hint on the unit, rmax is in Angstrom?
Also, concerning the n_radial, I would add the explanation from below: "(...) in message passing, which affects angular resolution"
Reply via ReviewNB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the note on units to the top and replaced the description ( it was wrong before, thanks for highlighting!)
| @@ -0,0 +1,742 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line #12. ax[1].set_ylabel("Density")
Should also be labeled 'counts' in order not to confuse.
Reply via ReviewNB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am using density for the forces, because there are so many force component but keep the non-normalized histogram for the energies, because the numbers still make sense to the reader.
|
In the ASE interface, we are setting the calculator in a loop. for _ in tbar:
atoms = system.ase.get()
atoms.calc = box.calcThe |
|
@PicoCentauri we are using the GMNN1 as implemented in APAX2 Footnotes |
|
Hi, I had another look and all my points seem well adressed. I just pushed quite a few of typo corrections and didactic rephrasings. From my side this is finished and can be used as tutorial! A few open points:
|
|
I can shorten this, but I think it includes some valuable information
I think this highlights again, that there are e.g. no springs between particles and if not enough information is given, the system can blow up. Maybe including another sentence to explicitly refer to the 2 A data might help?
Including the If this is information is to coarse and doesn't really benefit the model, I'll shorten the section as you've suggested? |
Yes, I think either you need to provide more explanations (like the one you provided above) or simplify the section... |
This PR adds the MLIP/Water Tutorial.
It is spilt into three parts
For testing until merged, you can use