Skip to content
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 "toolkit showcase" example #763

Merged
merged 27 commits into from
May 14, 2021
Merged

Add "toolkit showcase" example #763

merged 27 commits into from
May 14, 2021

Conversation

j-wags
Copy link
Member

@j-wags j-wags commented Nov 5, 2020

This is an example I've been meaning to add for a long time -- It's the notebook I go through at talks, which mainly features a way to go from a prepared ligand and protein all the way to a solvated simulation with ions. Several blocks in this notebook show how to do useful steps that haven't been documented anywhere else.

  • Address Smirnoff+Amber simulation #761 by adding a soup-to-nuts protein simulation setup example
  • Polish up text/documentation
  • Figure out how to handle nonstandard mdtraj and gromacs deps
  • Update changelog

@codecov
Copy link

codecov bot commented Nov 5, 2020

Codecov Report

Merging #763 (61105ee) into master (3b6b392) will not change coverage.
The diff coverage is n/a.

@mattwthompson mattwthompson added this to the 0.8.1 milestone Nov 17, 2020
@mattwthompson
Copy link
Member

mattwthompson commented Nov 30, 2020

@j-wags could you add these files to tracking? nevermind, I was in the wrong branch 🤦

  • CID_15513.pdb
  • minim.mdp
  • nvt.mdp
  • npt.mdp
  • md.mdp

Updates:
  * Update some markdown blocks
  * Make GROMACS output less verbose
  * Lint some code blocks

Experimental:
  * Split off contact function into file
  * Use type hints to indicate variable types
@mattwthompson mattwthompson modified the milestones: 0.8.1, 0.8.3 Dec 15, 2020
@mattwthompson mattwthompson modified the milestones: 0.8.3, 0.9.1 Jan 6, 2021
@mattwthompson mattwthompson modified the milestones: 0.9.1, 0.9.2 Feb 16, 2021
@Yoshanuikabundi
Copy link
Collaborator

We seem to be missing the img folder for this example - img/openforcefields.png, img/xkcd_charge.png, and img/dog_food.jpg. Can obviously remove the references to them but I like pictures! I'm guessing xkcd_charge.png is XKCD 567 but I'm clueless about the others.

@Yoshanuikabundi
Copy link
Collaborator

@j-wags
Copy link
Member Author

j-wags commented May 6, 2021

Notes from today's meeting:

  • Let's remove type hints
  • Remove -c omnia from openmmforcefields install instructions (also maybe just remove the install instructions since we'll now base this off openmmforcefields)
  • Since we're no longer using ParmEd for system combination, try using the constrained force field (openff-1.3.0.offxml) for the example. This should let us take 2fs timesteps. (this may crash at system export to GROMACS... Let's touch base if so)
  • add a note that the padding during water addition is too small for general use, it's just so that we can run a quick example
  • Remove ParmEd from water-clash resolution (maybe not needed since we'll solvate the protein+ligand together?)
  • Use constraints for hbonds throughout gromacs example

@Yoshanuikabundi

This comment has been minimized.

Using ommff simplifies the showcase substantially without
loss of function. With this commit, we also use the
constrained Parsley force field, reducing hidden magic
and increasing relevance. Prose and descriptions are not
yet updated. GROMACS now only requires `-maxwarn 1`, which
is due to a known openFF toolkit bug.
@Yoshanuikabundi
Copy link
Collaborator

Implemented the switch to openmmforcefields, at least in code. Turned out to be pretty simple, only needed to write ~30 lines of glue code to convert an OFFTK Topology to an OpenMM Topology, which now resides in a function in the utils.py file. This removes the need for the find_clashing_water function, so it seemed like a fair trade.

The resulting workflow is much simpler, and the OpenMM side of it doesn't need to touch ParmEd. I also got the GROMACS bit working without the extra warnings, and the whole thing now uses the standard Parsley 1.3.0 force field (rather than the constrained one).

Tomorrow I'll move the descriptions and prose around to update them for the new workflow and then we should be good to review.

@Yoshanuikabundi
Copy link
Collaborator

@j-wags Ready for review!

Switching to openmmforcefields really dramatically cuts down on fluff. The example now only uses ParmEd for visualization and conversion to GROMACS, everything else is done pretty much in OpenMM.

I do think it'd be cool if SMIRNOFFTemplateGenerator from OpenMMForceFields could take a OFFTK ForceField object instead of only being able to take a .offxml filename - that way we could use this simple, tidy, logical workflow even when we're playing with force fields, without having to write out to file. I also think that SMIRNOFFTemplateGenerator would be a very reasonable thing for the Toolkit itself to provide - it seems to be the only Python code we need OpenMMForceFields for, and users might prefer this workflow to directly creating OpenMM systems. It certainly makes it easier to combine things, which might be especially important for solvent-phase parameterization.

But I think I've done everything we planned here!

@Yoshanuikabundi Yoshanuikabundi marked this pull request as ready for review May 11, 2021 07:04
Copy link
Member Author

@j-wags j-wags left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-add openmmforcefields to environment --> Could add openff-toolkit-examples to binder reqs.

Try replacing openff_topology_to_openmm with offtop.to_openmm() (this worked during our call)

In the omm_forcefield.createSystem step, let's either:

  • add a warning box, stating that normally a SMIRNOFF FF contains information about the cutoff+constraints that should be used.
  • Load the openff-1.3.0 forcefield explicitly and reference parsley['Electrostatics'].cutoff (note though that this won't work for the openmm.app.PME or the hbond constraints)

We can keep from double-calling omm_forcefield.createSystem if we use a PDB file to go to NGLView right after we parameterize the complex:

from simtk.openmm.app import PDBFile
PDBFile.writeFile(modeller.topology, modeller.getPositions(), open('complex.pdb', 'w'))
view = nglview.show_file('complex.pdb')

Then the second createSystem and the ParmEd stuff can all go into the GROMACS section.

ParmEd can't handle constraints from openmmforcefields

should become

ParmEd's GROMACS exporter can't handle constraints from openmm

The Toolkit will always support all the functional forms in both the SMIRNOFF spec

should become

The Toolkit will strive to support all the functional forms in both the SMIRNOFF spec

Load the ligand molecule and the Parsley force field

could become

Teach OpenMM about the ligand molecule and the Parsley force field

Point the links to other examples to the folder containing the example notebook.

Pending the changes above, I approve this merge! Since I'm technically the author you'll need to self-approve and then merge, but you can let me know if there's trouble and I'll force it though.

Copy link
Collaborator

@Yoshanuikabundi Yoshanuikabundi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've implemented all the changes Jeff listed in his review, and I also changed the visualization steps to directly convert OpenMM Topology objects to MDTraj ones, which means we don't have to write PDB files to disk for them. I also wrote a readme for the example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants