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

Not only querying, but also modifying system parameters #18

Merged
merged 14 commits into from
May 1, 2023

Conversation

yuanqing-wang
Copy link
Contributor

@yuanqing-wang yuanqing-wang commented Apr 19, 2023

  • Modifying Parameters in System

in the same manner as in Espaloma
@yuanqing-wang yuanqing-wang changed the title Add OpenMM tutorials Add OpenMM tutorials for APIs of machine learning force field constructions Apr 19, 2023
@yuanqing-wang
Copy link
Contributor Author

cross-ref: #12

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.

These notebooks have a lot of overlap with existing ones. There's already a cookbook entry on inspecting system parameters. Is there a good reason to have two on the same thing? The other notebook is esoteric and sort of out of scope: using OpenMM to validate your own MD code that you've written separately. Mostly that isn't about OpenMM. The only part that's really about OpenMM is the discussion of how to query energy components, but we already have a cookbook entry on that.

Comment on lines 10 to 13
"Some machine learning force field construction schemes, such as [Espaloma](https://pubs.rsc.org/en/content/articlehtml/2022/sc/d2sc02739a), would output parameters which can be picked up by the parameter-holding `System` object of OpenMM, which could be further used in molecular dynamics (MD) simulations just like a vanilla `System`.\n",
"In this tutorial, we show how you can modify the parameters of `System` object so you can write your own machine learning force field API.\n",
"\n",
"This example is inspired by the [Espaloma deployment](https://github.com/choderalab/espaloma/blob/master/espaloma/graphs/deploy.py) implementation."
Copy link
Member

Choose a reason for hiding this comment

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

This description is overly specific. Nothing in this notebook is really specific to machine learning. You're just showing how to examine and change the parameters in a System. How about something like this?

Sometimes it is useful to examine the parameters in a System. For example, you might want to check what parameters a ForceField has assigned to a particular bond or angle. In other cases you may want to modify those parameters to make a bond behave differently from what the ForceField assigned.

@yuanqing-wang
Copy link
Contributor Author

@peastman how about I get rid of the separate "modify the parameters" one and simply append the existing "query the parameters" tutorial with ways to modify them in-place.

For the other one, I have seen so many people trying to using OpenMM to benchmark their own MM implementations. I feel like the community would benefit from a tutorial.

@yuanqing-wang
Copy link
Contributor Author

see 38035e4 where I combined the modifying parameter tutorial with the querying parameter tutorial

@yuanqing-wang yuanqing-wang requested a review from peastman April 21, 2023 17:30
@yuanqing-wang
Copy link
Contributor Author

would love your feedback! @peastman @sef43

Perhaps the second item is more suitable for a tutorial than a cookbook?

@sef43
Copy link
Contributor

sef43 commented Apr 24, 2023

I think the setting different parameters is a good addition to the existing cookbook entry!

I think as you have suggested the notebook for "Use OpenMM to Verify Your MM Functional Form Implementation" should be a separate tutorial. Can you remove it from this PR and make a new PR adding that as a tutorial, we can then review it properly in the new PR.

@yuanqing-wang yuanqing-wang changed the title Add OpenMM tutorials for APIs of machine learning force field constructions Not only querying, but also modifying system parameters Apr 25, 2023
@yuanqing-wang
Copy link
Contributor Author

@sef43 I've made the changes as requested. Will follow up with another PR for the separate notebook.

@yuanqing-wang yuanqing-wang requested a review from sef43 April 25, 2023 04:24
@yuanqing-wang
Copy link
Contributor Author

Separated PR here: #22

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.

Looks good! I made a few very minor suggestions.

@peastman
Copy link
Member

I just realized it's missing one very important part. Let's add the following at the end.

Modifying Force objects will affect any new Simulations or Contexts you create, but it will have no effect on ones that already exist. If you want your modifications to apply to an existing Simulation, you can copy them over by calling bonded.updateParametersInContext(simulation.context).

@yuanqing-wang
Copy link
Contributor Author

@peastman I've made the changes per your suggestions. I've also added the code to modify existing Context.

Comment on lines 152 to 155
" bonded.setBondParameters(\n",
" i, particle1, particle2, length, \n",
" 2666 * unit.kilojoules_per_mole/unit.nanometer**2\n",
" )"
Copy link
Member

Choose a reason for hiding this comment

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

No need to split this onto four lines. It makes the code less readable, and it can easily fit on a single line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! If we don't worry about the 80 char limit.

Copy link
Member

Choose a reason for hiding this comment

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

If someone wants to view the entry on a VT52 terminal, I admit it will be inconvenient. But I think we can say VT100 is the earliest terminal we support. :)

@yuanqing-wang
Copy link
Contributor Author

I've updated the PR accordingly.

@peastman
Copy link
Member

Just one unnecessary line that still needs to be removed. Then I think it will be ready to merge.

@yuanqing-wang
Copy link
Contributor Author

Just one unnecessary line that still needs to be removed. Then I think it will be ready to merge.

Sorry which unnecessary line?

@peastman
Copy link
Member

peastman commented May 1, 2023

bonded = [f for f in system.getForces() if isinstance(f, HarmonicBondForce)][0]

@peastman
Copy link
Member

peastman commented May 1, 2023

Looks good. Thanks!

@peastman peastman merged commit 83334ce into openmm:main May 1, 2023
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