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

Fix glycam ffxml and conversion script #180

Merged
merged 6 commits into from
Nov 10, 2021
Merged

Fix glycam ffxml and conversion script #180

merged 6 commits into from
Nov 10, 2021

Conversation

zhang-ivy
Copy link
Collaborator

This PR makes the following changes:

  • Fix problems with the external bonds for the following residues: terminal residues (e.g. NNLN, CNLN), OLP, and OME.
    • Terminal residue issue pointed out here
  • Update the initialization script according to OpenMM API updates introduced by Added GLYCAM openmm#3303

Requires: ParmEd/ParmEd#1206

A couple remaining issues:

  1. @peastman pointed out that the HYP residues does not use PDB v3 atom names here. The hydrogens.xml I generated relies on GLYCAM_06j-1.xml to determine the hydrogens for each residue. I'm not sure why the HYP hydrogen names do not match the ones in CHYP/NHYP. Could you share your script for converting the HYP hydrogen names to PDB v3 names? Or maybe you could just make the changes directly to the updated GLYCAM_06j-1.xml included in this PR and commit them?
  2. @peastman : I've fixed the bond perception issues for residues that I saw problems for and then chose a couple more random residues and checked thata those looked right. I'm not sure if there are more bond perception problems though, would you be able to randomly choose some residues and see if the external bonds look right?

@peastman
Copy link
Member

I didn't modify the atom names in the force field templates, since they aren't important. Atom names in templates are just arbitrary strings to make it easier to read.

The ones in the hydrogens file do matter, because those are the names that will be used in the topology. I modified the file by hand, not just to change the names but also to specify terminal residues correctly.

@zhang-ivy
Copy link
Collaborator Author

zhang-ivy commented Oct 28, 2021

I didn't modify the atom names in the force field templates, since they aren't important. Atom names in templates are just arbitrary strings to make it easier to read.

The ones in the hydrogens file do matter, because those are the names that will be used in the topology. I modified the file by hand, not just to change the names but also to specify terminal residues correctly.

Got it, thanks. I'll just leave the HYP hydrogen atom names as is then.

Just thought about this a little more -- for glycans, we match residues to their templates by names only, right? Does this means that its not essential that the external bonds in the templates are correct?

@peastman
Copy link
Member

It still matters. After the template matcher returns a match, ForceField still needs to topologically match the returned template to the residue so it can assign atom types and parameters. If the external bonds are different, that will fail and it will throw an exception.

@jchodera
Copy link
Member

jchodera commented Nov 6, 2021

@peastman: Can you help us get this merged in since the beta is now out?

@peastman
Copy link
Member

peastman commented Nov 8, 2021

What part do you need my help with?

@zhang-ivy
Copy link
Collaborator Author

This should be good to merge!

@mikemhenry mikemhenry merged commit 3783cc8 into master Nov 10, 2021
@mikemhenry mikemhenry deleted the fix-glycam branch November 10, 2021 23:47
@mikemhenry
Copy link
Collaborator

@jchodera should we cut a patch release (eg 0.10.1) so this fix is on conda-forge?

@jchodera
Copy link
Member

@jchodera should we cut a patch release (eg 0.10.1) so this fix is on conda-forge?

Apologies for missing this earlier! We've now got 0.11.0 out:
https://github.com/openmm/openmmforcefields/releases/tag/0.11.0

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.

4 participants