add umbrella sampling tutorial#24
Conversation
|
Thanks! This looks like a good start. Here are some thoughts on improvements. Could you add a paragraph at the beginning describing what umbrella sampling is and what it's used for? This is a tutorial, so we don't want to assume the reader already understands all the concepts. This is to teach them. For the same reason, it's best not to assume they know what abbreviations like PMF, SMD, and WHAM stand for. The first time one of them comes up, write it out in full. For example, "computing a potential of mean force (PMF)." Can you cite a publication for WHAM? They may want to learn about how it works. You first create a CustomBondForce with only a single bond, then wrap it in a CustomCVForce to compute a function of that bond length. Why not combine them and compute the function directly in the CustomBondForce? pullingForce = mm.CustomBondForce('0.5 * fc_pull * (r-r0)^2')It will run faster, and the code will be simpler. Does running the windows in parallel actually make much difference? Running multiple jobs at the same time on the same GPU generally doesn't work very well. It might help a bit on this system since it's so tiny, but on larger systems it's as likely to produce a slowdown as a speedup. It also risks running out of GPU memory or other resources, and it will fail if the device is set to exclusive mode. This tutorial has a lot of run-on sentences, could you break them up, it will make the text more readable? :) |
|
Thanks for the feedback! Agree with you about all the points about the text and explanations, will improve those.
I agree for this example that is simpler and more efficient, however, I did it that way to make this tutorial easier to generalise. It shows how you can define a collective variable (which could be some other more complex function), and then how you can add a biasing potential to that collective variable. I think it makes it easier for someone to take this example and use a different collective variable. Also is there a way to report the current value of
For this tiny example on my Macbook (platform is OpenCL on the M2 gpu i believe) it does speed up the total simulation time, but yes this is not general best practice, I will change this. |
|
There are some numbers for running OpenMM with MPS at openmm/openmm#3082. It gives some speedup when running two simulations at once, especially for small systems. But you still wouldn't want to run 24 simulations at once. |
|
This is ready for review again @peastman . I have added a lot more explanation |
peastman
left a comment
There was a problem hiding this comment.
It's looking good. I really like the introduction! I made a bunch of minor comments through the text. Here are a few more general ones.
There's a lot of inconsistency in the code formatting. Sometimes you put spaces around equals signs, sometimes not. Sometimes you even put a space on one side but not the other. Sometimes there are spaces after commas, sometimes not. Sometimes you use single quotes, sometimes double quotes. Sometimes variable names are lower case, sometimes upper case. This all makes the code less readable. Recommended formatting is:
- Always put spaces around assignment (
=) or comparison (==,<, etc.) operators, except when passing an argument by name. Optionally put them around other operators when you think it improves readability. In either case, an operator should always have spaces on both sides or neither side, never on one side but not the other. - Put spaces after commas.
- Use single quotes unless there's a good reason to use double quotes (for example, if a string contains a single quote that would otherwise need to be escaped).
- Variable names should always start with a lower case letter.
I flagged a lot of run-on sentences, but not nearly all of them. They hurt readability. Especially when describing complicated subjects, we want to avoid anything that adds unnecessary cognitive load for the reader.
|
Is this ready for another review? |
Yes it is thanks! |
peastman
left a comment
There was a problem hiding this comment.
It's looking good. One last round of very minor comments, and then it should be ready to merge.
|
I have addressed the comments |
|
Looks great. Go ahead and merge if it's ready. |
|
Neither this nor the one from #26 is showing up on the docs page. Do we just need to wait longer? It's been a few hours since they were merged. |
|
They are on the https://openmm.github.io/openmm-cookbook/dev/ page: They wont be on the https://openmm.github.io/openmm-cookbook/latest page untill we manually cut a release on the github repo |
|
and the main link https://openmm.github.io/openmm-cookbook is setup to redirect to https://openmm.github.io/openmm-cookbook/latest |
|
Ok. Should we make a release now? |
|
Yep I have done! |
This PR adds a umbrella sampling tutorial we produced. It uses a small test system and covers the entire workflow of steered MD setup, running umbrella windows, computing a PMF with WHAM. We (@jmichel80) thought it would be good for inclusion in the OpenMM tutorials.
Not sure how much overlap it has with #21