Overhaul treatment of nonbonded exceptions

[andrrizzi]

Continues #39 .

[andrrizzi]

Ok, I think I've found a way to handle correctly also exceptions between alchemical atoms and non-alchemical ones for decoupling.

In order to do this I had to treat the decoupled intra-alchemical interactions as exceptions in the unmodified nonbonded force.

This is a summary table showing how contributions are now distributed across forces

# ------------------------------------------------------------------------------------------------------------------------------------------------
# FORCE                                 | ANNIHILATED                                     | DECOUPLED                                            |
# ------------------------------------------------------------------------------------------------------------------------------------------------
# nonbonded_force (unmodified)          | all interactions nonalchemical-nonalchemical    | all interactions nonalchemical-nonalchemical         |
#                                       | all exceptions nonalchemical-nonalchemical      | all exceptions nonalchemical-nonalchemical           |
#                                       |                                                 | all interactions alchemical-alchemical               |
#                                       |                                                 | all exceptions alchemical-alchemical                 |
# ------------------------------------------------------------------------------------------------------------------------------------------------
# sterics_custom_nonbonded_force        | sterics interactions alchemical-allatoms        | sterics interactions alchemical-nonalchemical        |
# ------------------------------------------------------------------------------------------------------------------------------------------------
# electrostatics_custom_nonbonded_force | electrostatics interactions alchemical-allatoms | electrostatics interactions alchemical-nonalchemical |
# ------------------------------------------------------------------------------------------------------------------------------------------------
# sterics_custom_bond_force             | sterics exceptions alchemical-allatoms          | sterics exceptions alchemical-nonalchemical          |
# ------------------------------------------------------------------------------------------------------------------------------------------------
# electrostatics_custom_bond_force      | electrostatics exceptions alchemical-allatoms   | electrostatics exceptions alchemical-nonalchemical   |
# ------------------------------------------------------------------------------------------------------------------------------------------------

Now I reactivated the test compareEnergies which is failing. I'm now augumenting check_interacting_energy_components() test to dissect the contribution and understand where we are failing.

For some reason when I run this tests on my local laptop and on the cluster I get (sometimes) Abort trap: 6. I'll try to do reinstall conda from scratch and see if I can get rid of it since Travis doesn't complain.

[jchodera]

In order to do this I had to treat the decoupled intra-alchemical interactions as exceptions in the unmodified nonbonded force.

This sounds correct to me.

This is a summary table showing how contributions are now distributed across forces

Great table!

Now I reactivated the test compareEnergies which is failing. I'm now augumenting check_interacting_energy_components() test to dissect the contribution and understand where we are failing.

Fantastic. I think this is exactly what we need to be doing.

For some reason when I run this tests on my local laptop and on the cluster I get (sometimes) Abort trap: 6. I'll try to do reinstall conda from scratch and see if I can get rid of it since Travis doesn't complain.

Are these energy comparison tests or actual dynamics simulations? This is likely the CPU platform running into a segfault.

[andrrizzi]

This is likely the CPU platform running into a segfault.

After reinstalling miniconda from scratch, I isolated the code giving me Abort trap: 6 to the line where we obtain the state. But is seems to be quite random, it doesn't happen all the time (which luckly means I can keep debugging alchemy locally).

[jchodera]

Does it abort if you just get the positions? Or is it the energy calculation?

If the positions are OK, you can serialize the system and state out to XML and then submit that as a bug report to openmm.

[andrrizzi]

If the positions are OK, you can serialize the system and state out to XML and then submit that as a bug.

Right! I didn't think about that, thanks. I think it's the energy. I'll open an issue on openmm soon.

[andrrizzi]

I've reworked the implementation of nonbonded force with the scheme suggested by @Lnaden, and rewrote the test check_interacting_energy_components() to check all different contributions by atom group (non-alchemical, alchemical, interface) and type (sterics, electrostatics). It should be pretty comprehensive.

I tried to implement this in the clearest way possible and added a lot of comments, but let me know if you think there's something that can be improved/changed.

I'll now extend check_interacting_energy_components() to test also for other forces other than NonbondedForce.

[andrrizzi]

I'll now turn off the PME test cases fed to check_interacting_energy_components() since we don't expect them to work yet. I'll then clean up tests to be sure we have not left out anything while making temporary changes for debugging, and then I think this is ready to merge if all tests will pass.

[jchodera]

I'll now turn off the PME test cases fed to check_interacting_energy_components() since we don't expect them to work yet. I'll then clean up tests to be sure we have not left out anything while making temporary changes for debugging, and then I think this is ready to merge if all tests will pass.

Can we just disable the electrostatics part of the test? We should leave the test that checks alchemical overlap for PME.

[andrrizzi]

Can we just disable the electrostatics part of the test?

Yes! Even better, thanks.

[andrrizzi]

One of the tests (Alanine dipeptide in vacuum) is failing because of the OpenMM bug that assign complete interaction when one of the two sets given to addInteractionGroup(set1, set2) is empty (pandegroup/openmm#1554).

Is it ok if I deactivate the test until the new OpenMM gets released?

[jchodera]

One of the tests (Alanine dipeptide in vacuum) is failing because of the OpenMM bug that assign complete interaction when one of the two sets given to addInteractionGroup(set1, set2) is empty (pandegroup/openmm#1554). Is it ok if I deactivate the test until the new OpenMM gets released?

Yes!

[jchodera]

The 16 force limit was probably just my misunderstanding of the OpenMM limitation!

[andrrizzi]

Great! I'll fix the last tests today and merge it if there are no objections.

[Lnaden]

none from me

[andrrizzi]

Hmm, this Travis error doesn't look like a timeout and happens at random lines inside check_interacting_energy_components (judging from the print statements).

According to Travis docs, it is probably exhausting the memory which is consistent with the problem I've been having when contexts get continuously created and deleted.

Those tests pass locally on my machine so, if we are in a rush to merge this PR, I can temporarily deactivate them, see if Travis pass all other tests, and then reactivate them and merge. I'll try to debug the OpenMM problem over the weekend.

[andrrizzi]

Just to give references to the issues I'm referring to: pandegroup/openmm#1588, and probably also choderalab/yank#397 is related to this.

[jchodera]

Yeah, I'm not sure what to make of this error. I think we might need to wait for the memory leak in https://github.com/pandegroup/openmm/issues/1588 to be tracked down.

[jchodera]

Maybe we should just mark the test as slow for now and be sure to test locally until the openmm memory leak is found.

[andrrizzi]

Sounds good. Local tests pass on my machine, so if the other tests are successful on travis I'll merge this.

[jchodera]

Woohoo! Tests pass!

[andrrizzi]

Great! Merging.