-
Notifications
You must be signed in to change notification settings - Fork 5
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 tutorial for MD simulations #97
Conversation
I added this as a tutorial, but could also be a cookbook instead. |
Just cycling this PR in the hope that reviewnb kicks in. |
@@ -0,0 +1,369 @@ | |||
{ |
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.
[enhancement]
What would be super useful here is some kind of rough table of how the protocol works like
- Input handling using gufe
- Parameterization using OpenMMForceFields & OpenFF
- OpenMM object creation
- Minimization
- NVT equilibration (if not gas phase)
- NPT equilibration (if not gas phase)
- NPT production (if not gas phase)
The table could have the columns "step", "software used", and "outputs". That way folks know what files come out of which steps.
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.
Added a table describing these steps.
@@ -0,0 +1,369 @@ | |||
{ |
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 can probably go without the "when creating the OpenMM simulation objects" since those aren't exposed to users.
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.
Removed this
@@ -0,0 +1,369 @@ | |||
{ |
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.
1)
The bullet points go from 8 to 10 (also there's a weird indentation for 10).
If you felt like it, it might be worth seeing if a table format might be cleaner here (but that's really just if you want to play around with formatting).
2)
"not the entire system with all the atoms should be saved" - a bit nitpicky, but this is a bit difficult to read because of the "not". Maybe "Special output indices can be defined to select which portions of the system should be saved" (it's the same but without the "not"). We should also make it clear as to what the default is (I think it's "not water"?)
It would be good to highlight the specific attributes that would need changing if possible. e.g. output_indices
.
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.
That's a good idea! I moved the bullet points into a table, changed the sentence to your suggestion and changed some of the descriptions to highlight the code attributes. I hope this is intuitive for the user.
@@ -0,0 +1,369 @@ | |||
{ |
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 #6. settings.simulation_settings.equilibration_length_nvt = 0.01 * unit.nanosecond
[enhancement]
To help the reader, it may be worth adding inline comments here about what these are doing?
Something along the lines of "# setting the nvt equilibration length to 0.1 ps"
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.
Added this!
@@ -0,0 +1,369 @@ | |||
{ |
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 be good to give a final look at what files were created (i.e. the output of an ls
), and maybe a comment about if you set protocol_repeats to >1 then you will get N full replicates of these outputs?
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.
Added this!
@@ -0,0 +1,369 @@ | |||
{ |
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.
1)
I'd remove the output from this cell folks might get confused as to why it's CPU only :P
2)
[enhancement]
You could possibly add a "performance consideration" section - something along the lines of "if you're running gas phase, then we suggest setting OPENMM_CPU_THREADS
to 1".
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.
Added this!
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 so much for working on this @hannahbaumann !
I've added some comments via reviewnb (it's an app that sits on top of github to help augment jupyter notebook reviews). If you click on the Reply via ReviewNB
link then it'll take you to a place where you can more easily review comments.
Anything that's tagged [enhancement]
is me saying "this would be nice to have but not an obligation if you don't want to think about it here".
@@ -0,0 +1,369 @@ | |||
{ |
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.
Oh yes, that would be great if you can make a pretty picture for this!
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.
added :)
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, this looks great!
@@ -0,0 +1,369 @@ | |||
{ |
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.
short comment on: why is this useful? what can it be used for?
- Maybe you only want to see quickly the dynamics of a system.
- or want to pre equilibrate a given complex in order to use it for FEs later.
Reply via ReviewNB
@@ -0,0 +1,369 @@ | |||
{ |
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 #1. transformation = openfe.Transformation(
could we use a NonTransformation
gufe class here?
That way we could get rid of SystemB :)
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.
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, I changed this!
Hi @hannahbaumann this is a really cool PR, I like it a lot! I will try it out know directly on my machine and let you know how things go :) |
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
@@ -0,0 +1,428 @@ | |||
{ |
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 doesn't seem to be rendering properly on my end - are you getting the same thing?
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.
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.
Yeah it's showing as a broken table on preview (by comparison the showcase notebook looks fine).
The second table now looks fine after your latest commit though!
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.
Quick test in #116 - looks like the table is missing a |
at the end of each row starting from the third row? Adding those in makes the render work.
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 for addressing all the comments!
Only thing left on my end is that the tables don't seem to render properly - I'm not sure if it's just me though.
Just testing table stuff - please ignore
Merged in your fix of the table! |
Only thing missing would be a pretty complex picture, if @RiesBen you have one, it would be great if you can add it, but this is something that would be nice to have but not required for merging. |
a complex picture :)
testing adding the complex image
@mikemhenry do you have any idea what's going on with that container build step? |
@richardjgowers you are assigned here - can we go ahead and merge this so we can resolve OpenFreeEnergy/openfe#781? |
No description provided.