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

The first tutorial of OpenMM-Torch with NNPOps #62

Merged
merged 10 commits into from
Mar 1, 2022
Merged

Conversation

raimis
Copy link
Contributor

@raimis raimis commented Feb 2, 2022

  • Write a tutorial
  • Add an automated test
  • Add a link to README.md

Solves #59

Try on Colab: https://colab.research.google.com/github/raimis/openmm-torch/blob/example/tutorials/openmm-torch-nnpops.ipynb

@raimis raimis self-assigned this Feb 2, 2022
@raimis raimis linked an issue Feb 2, 2022 that may be closed by this pull request
@raimis raimis marked this pull request as ready for review February 11, 2022 16:53
@raimis
Copy link
Contributor Author

raimis commented Feb 11, 2022

The first tutorial of OpenMM-Torch is done.

There are two outstanding issue:

Anyway, an imperfect tutorial is better than no tutorial.

Copy link
Member

@peastman peastman left a comment

Choose a reason for hiding this comment

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

This is really nice!

"# Create a simulation and set the initial positions and velocities\n",
"simulation = Simulation(ala2.topology, ala2.system, integrator)\n",
"simulation.context.setPositions(ala2.positions)\n",
"# simulation.context.setVelocitiesToTemperature(temperature) # This does not work (https://github.com/openmm/openmm-torch/issues/61)\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this work now that #61 is closed?

Copy link
Member

Choose a reason for hiding this comment

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

It's fixed in the main branch, but the fix hasn't gotten into a release yet.

@jchodera
Copy link
Member

Would be great to get feedback from @dominicrufa too!

Copy link
Member

@jchodera jchodera left a comment

Choose a reason for hiding this comment

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

This is really great! Just had some minor comments.

tutorials/openmm-torch-nnpops.ipynb Outdated Show resolved Hide resolved
"source": [
"# Note: Remove \"mmh\" when NNPOps in available on conda-forge (https://github.com/openmm/NNPOps/issues/26)\n",
"# Note: \"cudatoollit=11.2\" is need to override a pinning in conda-colab\n",
"!conda install -q -c conda-forge -c mmh \\\n",
Copy link
Member

Choose a reason for hiding this comment

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

If we use mamba instead of conda here, it can speed up installs significantly (presuming we also used mambaforge above).

@mikemhenry : Are we ready to drop the -c mmh yet?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not yet, if we can't drop it by the time we want to merge the tutorial, we can make an issue to drop it so I won't forget once we get NNPOps on conda-forge

"# Get the system of alanine dipeptide\n",
"ala2 = openmmtools.testsystems.AlanineDipeptideVacuum(constraints=None)\n",
"\n",
"# Remove MM forces\n",
Copy link
Member

Choose a reason for hiding this comment

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

What about refactoring the process of removing MM forces and creating/adding the ML potential into a small function folks can copy-and-paste into their own code?

For example, this code could be bundled together:

def replace_mm_with_nnp(system, topology, NNP):
    """
    Create an instance of the specified machine learning potential and replace all MM forces with the ML potential.

    """
    # Remove MM forces
    while system.getNumForces() > 0:
        system.removeForce(0)

    # Assert no forces or constraints remain
    assert system.getNumConstraints() == 0 
    assert system.getNumForces() == 0

    # Get atomic numbers
    atomic_numbers = [ atom.element.atomic_number for atom in topology.atoms() ]

    # Create an instance of the model
    nnp = NNP(atomic_numbers)
   
    # Save the NNP to a file and load it with OpenMM-Torch
    # TODO: Use a temporary file instead
    pt.jit.script(nnp).save('model.pt')
    force = TorchForce('model.pt')

    # Add the NNP to the system
    system.addForce(force)
    assert ala2.system.getNumForces() == 1 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As commented below, this a low-level tutorial. All the convenience functionality should be in OpenMM-ML.

" super().__init__()\n",
"\n",
" # Store the atomic numbers\n",
" self.atomic_numbers = pt.tensor(atomic_numbers).unsqueeze(0)\n",
Copy link
Member

Choose a reason for hiding this comment

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

How does this work for atoms that should not have their potential computed by the NNP? Would we use atomic numbers of 0 for those atoms? For example, if we wanted to only compute ligand energies for a solvated protein:ligand system?

Copy link
Member

Choose a reason for hiding this comment

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

I think that's beyond the scope of this first tutorial. Mixing ML with MM could be a more advanced tutorial. Or we could just refer people to OpenMM-ML, which has code to automate it.

Copy link
Member

Choose a reason for hiding this comment

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

It seems weird not to even mention how it would work, since this is a major use case. If folks wanted to just simulate only ML, why would they use OpenMM?

I agree a separate tutorial would be useful!

What about periodic ML systems? Do we support those? Or just non-periodic systems?

Maybe we could have three tutorials:

  • Vacuum ML
  • Periodic ML
  • Hybrid ML/MM periodic

Copy link
Member

Choose a reason for hiding this comment

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

Periodic systems just take a minor change to the model. See https://github.com/openmm/openmm-torch/blob/master/README.md.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intention of this tutorial is to demonstrate how to connect 3 dots: OpenMM-Torch, TorchANI, and NNPOps. All the advanced stuff should be done at a high-level with OpenMM-ML.

@raimis
Copy link
Contributor Author

raimis commented Mar 1, 2022

No more comments, so I merge the tutorial. We could revise it when there will be more tutorials in place and we will have a better idea.

@raimis raimis merged commit e6a3179 into openmm:master Mar 1, 2022
@jchodera
Copy link
Member

jchodera commented Mar 2, 2022

@dominicrufa: Can you provide some feedback too?

@mikemhenry
Copy link
Collaborator

@raimis nnpops is now on conda-forge 🎉 mamba info nnpops So you can drop the -c -mmh now

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.

Add example of using NNPOps with openmm-torch?
4 participants