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: correct usage of nonbond energy for close contacts #368

Merged
merged 33 commits into from Mar 17, 2023

Conversation

DaniBodor
Copy link
Collaborator

@DaniBodor DaniBodor commented Feb 22, 2023

Previously, a distinct set of parameters was used to calculate Van der Waals energy, depending on whether atoms are part of the same residue or not. Electrostatic energy was calculated in the same way for any atom pair.

In the current PR, this is changed such that:

Tests were adjusted and added according to above rules.

Also, nomenclature has been improved for:

  • nomenclature for vanderwaals and electrostatic energy has been unified
  • nomenclature for inter_ and intra_ parameters adjusted to _main and _14 parameters in the parser. Not sure I am totally happy with this nomenclature, so open to suggestions.

@DaniBodor
Copy link
Collaborator Author

@cbaakman , do you think we need to create a test to check that the correct parameters are used?

Do you have an idea how to do this in a simple way?

@cbaakman
Copy link
Collaborator

cbaakman commented Feb 24, 2023

@cbaakman , do you think we need to create a test to check that the correct parameters are used?

Do you have an idea how to do this in a simple way?

We could at least check the output of the function _intra_partners, use a well known pdb structure with well known binding partners.

@cbaakman
Copy link
Collaborator

I see that numpy is still used here. I wonder whether we could use torch instead.

@DaniBodor
Copy link
Collaborator Author

@cbaakman , do you think we need to create a test to check that the correct parameters are used?
Do you have an idea how to do this in a simple way?

We could at least check the output of the function _intra_partners, use a well known pdb structure with well known binding partners.

I actually since noticed that in PR #348 , you added tests for exactly this (although the function used there is not part of the PR, maybe you forgot to push that file?). I am guessing you looked at the structure by eye and found residues at different covalent bonding distances to test individually. I can indeed use those same residues to test the output of _intra_partners as you suggest.

It would be even nicer to test whether the output of _get_vdw_energy is correct, but that would be slightly more complex. I'll take a look whether I can make that work as well.

@DaniBodor
Copy link
Collaborator Author

I see that numpy is still used here. I wonder whether we could use torch instead.

What would the advantage be? Speed?

@cbaakman
Copy link
Collaborator

What would the advantage be? Speed?

Yes, we could use the GPU. As I understand, numpy is always CPU.

@DaniBodor DaniBodor requested review from cbaakman and LilySnow and removed request for cbaakman March 13, 2023 16:06
@DaniBodor
Copy link
Collaborator Author

@cbaakman , can you please re-review this, as it is now a completely different PR than it was the first time you looked at it.

@DaniBodor DaniBodor changed the title fix: correct usage of intra vs inter parameters for vdw energy fix: correct usage of nonbond energy for close contacts Mar 16, 2023
@DaniBodor
Copy link
Collaborator Author

DaniBodor commented Mar 16, 2023

In case you review this while the builds are still failing, please ignore that. It is a conflict between newest version of pytorch and pytorch geometric, and is being dealt with in a separate PR (#386)

@LilySnow
Copy link
Collaborator

For the test, we should compare with real force field calculations.

For example, for l LYS A 3 C TYR B 72 CZ 8.363386
charge1= 0.500; charge2= 0.265; dist = 8.363386

the CNS calculated Eelec energy using OPLS force field is:

0.1 * (0.50.265/8.363386)(1.0 - (8.3633862/8.52))**2 * 332.0636
0.0005348836008092561

Also, for the attached pdb file, the inter-chain energy should be: Evdw = -33.1338 with hydrogens (or -29.729 without hydrogens), Eelec = -8.87.

1A2K_3002.pdb.zip

I think we should move to openMM soon to avoid all these...

@DaniBodor
Copy link
Collaborator Author

For the test, we should compare with real force field calculations.

For example, for l LYS A 3 C TYR B 72 CZ 8.363386 charge1= 0.500; charge2= 0.265; dist = 8.363386

the CNS calculated Eelec energy using OPLS force field is:

0.1 * (0.5_0.265/8.363386)_(1.0 - (8.3633862/8.52))**2 * 332.0636
0.0005348836008092561

Also, for the attached pdb file, the inter-chain energy should be: Evdw = -33.1338 with hydrogens (or -29.729 without hydrogens), Eelec = -8.87.

1A2K_3002.pdb.zip

I think we should move to openMM soon to avoid all these...

Given that we are planning to move to openMM within a reasonable amount of time, do you think it's ok to leave the tests as they are for now, rather than spend time on doing it as you suggest here?

Copy link
Collaborator

@cbaakman cbaakman left a comment

Choose a reason for hiding this comment

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

Nicely done!

@DaniBodor DaniBodor merged commit 57dd25d into main Mar 17, 2023
5 checks passed
@DaniBodor DaniBodor deleted the 357_vanderwaals_HiLo_fix_dbodor branch March 17, 2023 17:03
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.

feat: redefine potential energy terms between atom pairs, depending on bond distance
3 participants