[ WIP ] Add tutorial for membrane RBFE calculation #286
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
|
hannahbaumann
changed the title
| @@ -0,0 +1,1153 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #25. settings=RelativeHybridTopologyProtocol._adaptive_settings(
Do we not need to add the lipid force field to the settings or is adaptive doing this? Also this is another place we are exposing _adaptive to the world do we need to add a public entry point?
Reply via ReviewNB
There was a problem hiding this comment.
i currently added the lipid forcefields in the _adaptive_settings as well, but we can think about changing that later.
| @@ -0,0 +1,1153 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #43. network = openfe.AlchemicalNetwork(transformations)
Woow so many warnings we need to remove a lot of these!
Reply via ReviewNB
There was a problem hiding this comment.
Already done - Hannah's branch isn't up to date with main.
| @@ -0,0 +1,1153 @@ | |||
| { | |||
There was a problem hiding this comment.
Two out of three of these imports warning seems to be some kind of problem with our typing vendor in gufe. I have opened an issue.
Reply via ReviewNB
| @@ -0,0 +1,1153 @@ | |||
| { | |||
There was a problem hiding this comment.
For this example, rather than separating this in a separate step, I think it will be more useful to the readers if you just do these calls in-place, i.e. modeller = openmm.app.Modeller(protein.to_openmm_topology(), protein.to_openmm_positions().
With a little bit of formatting, that will look a lot less overwhelming.
Reply via ReviewNB
| @@ -0,0 +1,1153 @@ | |||
| { | |||
There was a problem hiding this comment.
This is too unweidly for users in my opinion, I think OpenFreeEnergy/gufe#568 will need to be completed.
Reply via ReviewNB
There was a problem hiding this comment.
That would be great!
| @@ -0,0 +1,1153 @@ | |||
| { | |||
There was a problem hiding this comment.
Generally would not recommend overwriting an existing file in your example, that's going to lead users to do bad things on their end - I would instead suggest you set a new file name for it.
Reply via ReviewNB
There was a problem hiding this comment.
db_complex = 'a2a/a2a_complex.pdb' is supposed to be a new file! I will remove it from the PR.
| @@ -0,0 +1,1153 @@ | |||
| { | |||
There was a problem hiding this comment.
Could you somewhere make a statement about which of these would be applicable to the "merged PDB" example shown above? (give that most Maestro users would be going down that path).
numpy array using OpenFF units (see below)
This is the wrong level of details at this point, here I would just say "box vectors in OpenMM format". Then in the section below, I would precise that it has to be a united array.
Reply via ReviewNB
There was a problem hiding this comment.
Added a maestro note and changed the box vector desciption.
| @@ -0,0 +1,1153 @@ | |||
| { | |||
There was a problem hiding this comment.
Add some details on how this is done? Any caveats that one might need to consider (i.e. making sure that the system is wrapped / not broken across periodic images)?
Reply via ReviewNB
| @@ -0,0 +1,1153 @@ | |||
| { | |||
There was a problem hiding this comment.
Add some details on the types of lipid forcefields that are currently supported (i.e. I know there are some that won't work, which currently do work and which should one avoid?)
Reply via ReviewNB
There was a problem hiding this comment.
Added a comment here. I haven't tried out ff other than lipid17, also commented on that.
| @@ -0,0 +1,1152 @@ | |||
| { | |||
There was a problem hiding this comment.
Do we need to say anything about how we expect the membrane to be aligned with the coordinate system so that the barostat applies the pressure correctly?
Reply via ReviewNB
3 participants
No description provided.