-
Notifications
You must be signed in to change notification settings - Fork 187
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
Gibbs Ensemble scripts #4243
Gibbs Ensemble scripts #4243
Conversation
Would you mind investigating using MD moves instead of MC moves to propagate the internal states of each system. Just instead of moving 1 particle and computing exp(-beta Delta E_pot) in an MC step, run x MD steps (e.g. 10-100) and compute exp(-beta Delta E_pot) from before and after running MD. Running MD will require that you initialize to the velocities with a Maxwellian distribution like e.g. in
You will also have to avoid inserting particles closer than e.g. 0.9 sigma to avoid instabilities in the MD (see
|
I agree that MD moves will likely accelerate things. But let’s keep this Pr to the basics. Once it is in, we can extend upon it.
|
For now I updated some comments, added docstrings and copyright disclaimers. Things that still need to be done:
|
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.
Thank you. Just two minor issues left
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.
Here is a review from the point of view of code maintainability. I'm not sure the shell script can be tested, unless we can extract the number of steps as an external variable and call the script from a python subprocess.
@jhossbach please apply suggested changes with option "Add suggestion to batch", we have limited CI resources :-) |
I like using handles to store the particle data, but why not use it for the other methods as well? This way one doesn't have to store the data with the to_dict() >method
The handle is a proxy to the actual particle in the Espresso system. Once you delete the particle, the handle is meaningless. Hence, just storing the handle is not enough information to revert a removal of a particle. You have to store the actual particle properties (as a copy), before removing the particle.
|
With fe1f069 I did a last sweep over the plot script. Please review it again.
|
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.
looks very good, i just have added a few minor comments
From my side, we're good, here. |
I'll have another look and rebase the PR. There are 68 commits, many of which are fixups and reverts. |
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.
A few suggestions. They have been addressed in 684a12d
The PR was rebased to remove accidentally committed files from the git history and squash the fixups. I've also added copyright headers and wrapped long lines (see |
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 from my side
@RudolfWeeber don't forget to change your review from "Requested changes" to "Accepted", then it can be merged |
This is an updated version of #3903
Several updates were performed:
This updated way of communication between processes is far more efficient than using TCP connections which is either a result of an increase in latency or the multiprocessing module uses internal methods to efficiently use nonblocking communication between the processes.
The simulation has the following structure: