-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add Multiparticle Collision Dynamics tutorial #122
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by 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.
Thanks!
I pushed a commit that should let the CI checks run. I also have a few suggestions to improve some of the code.
You also need to run pre-commit:
pre-commit run --all-files
ruff check . --fix
will apply automatic fixes. To correct the unused loop variable check, replace i
with _
.
@@ -0,0 +1,505 @@ | |||
{ |
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.
For style consistency with other tutorial, replace the matplotlib import with:
%matplotlib inline matplotlib.style.use('ggplot') import matplotlib_inline matplotlib_inline.backend_inline.set_matplotlib_formats('svg')
Reply via ReviewNB
@@ -0,0 +1,505 @@ | |||
{ |
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.
This section jumps straight to setting all of the needed methods and forces - but these have not yet been introduced.
Add a paragraph at the beginning of this section that briefly explains what the mpcd.Integrator
does and list the methods it applies (streaming_method, collision_method, solvent_sorter, mpcd_particle_body_force). It is also unclear what the triggers are or how they are used. Please add pseudocode (like that I wrote for Simulation.run: https://hoomd-blue.readthedocs.io/en/v4.7.0/package-hoomd.html#hoomd.Simulation.run) that outlines the mpcd integration logic.
Also, why is it "solvent" sorter but "mpcd_particle" body force? Are these referring to two different things? If not, perhaps we should modify the MPCD code in HOOMD before release and give them the same name.
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.
You are right, I thought I had grepped and replaced all the uses of solvent in the API, but I must have missed this. It should be mpcd_particle_sorter
. I will open an issue and PR to fix before release
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.
It might also be helpful to add this pseudocode in the Integrator
documentation itself.
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.
Sorry, following up on this: I was mistaken, and I did make this correction in the code. This is a typo in the tutorial.
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.
Good idea. The psedocode belongs in the documentation and the tutorial should give an overview of the steps.
@@ -0,0 +1,505 @@ | |||
{ |
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.
Please rewrite this with the object oriented matplotlib API for style consistency with the other tutorials, and to promote the use of modern matplotlib syntax (see https://hoomd-blue.readthedocs.io/en/v4.7.0/tutorial/00-Introducing-HOOMD-blue/07-Analyzing-Trajectories.html for an example)
Reply via ReviewNB
@@ -0,0 +1,511 @@ | |||
{ |
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.
@@ -0,0 +1,511 @@ | |||
{ |
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. snapshot = hoomd.Snapshot().from_gsd_frame(
Use hoomd.Snapshot.from_gsd_frame
- it is a class method.
Reply via ReviewNB
@@ -0,0 +1,511 @@ | |||
{ |
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.
The prose states "20 collision steps", but the code has period=20
. Do you mean "timesteps" or period=integrator.collision_method.period * 20
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.
This is a typo, steaming is performed every collision (20 timesteps). Use period=integrator.collision_method.period
to help clarify this
@@ -0,0 +1,511 @@ | |||
{ |
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.
@@ -0,0 +1,511 @@ | |||
{ |
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.
Rename dump
to gsd_writer
for consistency with other tutorials. dump
is not a verb used to describe a writer in HOOMD and it carries many negative connotations.
Reply via ReviewNB
@@ -0,0 +1,511 @@ | |||
{ |
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 #7. sim.run(200000, write_at_start=True)
Remove write_at_start=True
. Tutorials should promote the use of default behavior. Users can customize their own scripts as desired.
Reply via ReviewNB
@@ -0,0 +1,511 @@ | |||
{ |
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.
This whole section can be shortened by using Freud to compute the MSD: https://freud.readthedocs.io/en/latest/modules/msd.html
Reply via ReviewNB
Click the "ReviewNB" button to see these comments in the context of the rendered notebook. |
And now the CI tests are failing due to a pybind version requirement. I will fix this on another branch. |
This has been updated as requested. I added in a paragraph explaining the MPCD technique and integrator, but will add the suggested pseudocode to the |
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.
Thanks! I have one final suggestion to improve this tutorial.
@@ -0,0 +1,1360 @@ | |||
{ |
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.
This is not a Snapshot
, it is a gsd Frame
. What is the reason for the round trip through the filesystem? As you require a Snapshot
below, why not start by placing particles in a Snapshot
?
I would suggest changing gsd.hoomd.Frame()
to hoomd.Snapshot
, then you can remove the GSD file write cell and the text description that goes with it.
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 asked @clpetix to do it this way to demonstrate how to initialize an MPCD simulation from a GSD file. We might expect users to have made their initial configuration for the HOOMD particles in a GSD file elsewhere, but GSD cannot currently store MPCD particles so we cannot directly initialize from one. I hope we can add that ability later, but the current solution is to create a hoomd.Snapshot
from the GSD file, then add the MPCD particles to the snapshot before initialization.
Is it allowed to include a GSD file with a tutorial? That would make this intent clearer, as the code for creating the HOOMD particles is kind of boilerplate covered by other tutorials.
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.
Understood. I do demonstrate writing GSD files first in the other tutorials because this is a common usage pattern in production (initialization and simulation in separate files / methods).
We should at least correct the reference to Snapshot
.
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.
OK! We can definitely clarify that this is a Frame
.
Is it possible for us to include the premade GSD file itself with this tutorial (and omit the code that makes it from the tutorial), or do you prefer that the code for creating the GSD file lives in the notebook? If we keep the code, we should add a little discussion to make it clearer why we're doing it this way, as it probably seems unnecessarily complicated to someone who is already familiar with hoomd.Snapshot
.
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.
We should keep the code. I find that users often download individual .ipynb
files and run them. They get confused when there are other files required to run the notebook.
One solution might be to move the gsd file creation up into the "Boilerplate" section. Or perhaps to its own section.
@@ -0,0 +1,1360 @@ | |||
{ |
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.
@@ -0,0 +1,1360 @@ | |||
{ |
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.
Thanks!
Description
Adds a tutorial showing how to use the MPCD module. It uses pressure driven flow of MPCD particles between parallel plates and MD particles in an MPCD solvent as examples.
Resolves: N/A
Checklist: