Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Thanks @jthorton these look great! I did a first pass looking through the 4 tests you had mentioned. Some of my comments may just be things that I didn't understand correctly in the htf =)
|
|
||
| for end_state, ref_system, ref_top, pos in [ | ||
| (0, htf._old_system, htf._old_topology, htf._old_positions), | ||
| (1, htf._new_system, htf._old_topology, htf._new_positions) |
There was a problem hiding this comment.
Are the topologies the same or should this be the htf._new_topology?
There was a problem hiding this comment.
They are the same due to the transforms I chose but I'll update it to use the new_topology
|
|
||
| for end_state, ref_system, ref_top, pos in [ | ||
| (0, htf._old_system, htf._old_topology, htf._old_positions), | ||
| (1, htf._new_system, htf._old_topology, htf._new_positions) |
There was a problem hiding this comment.
Same as below, I'm probably just missing something, or should this be the _new_topology?
| ref_energy = ref_state.getPotentialEnergy().value_in_unit(unit.kilojoule_per_mole) | ||
| # energies should be the same | ||
| # this is only true if we correctly interpolate the 1-4 interactions involving dummy atoms | ||
| assert pytest.approx(hybrid_energy) == ref_energy |
There was a problem hiding this comment.
Maybe put approx on the right hand side, e.g.
assert hybrid_energy == pytest.approx(ref_energy, rel=1e-5)
| platform=platform | ||
| ) | ||
|
|
||
| for end_state, ref_system, ref_top, pos in [ |
There was a problem hiding this comment.
Would you also have to set the NoCutoff for the reference system as above for the hybrid system?
There was a problem hiding this comment.
Yes we should to make the test more stable incase the default is not nocutoff in future which it seems to be.
| ref_state = ref_simulation.context.getState(getEnergy=True, groups={1}) | ||
| ref_energy = ref_state.getPotentialEnergy().value_in_unit(unit.kilojoule_per_mole) | ||
| # energies should be the same | ||
| # this is only true if we correctly interpolate the 1-4 interactions involving dummy atoms |
There was a problem hiding this comment.
Do you also want to check for a non-zero energy, as in the PME test?
|
|
||
| # set the nonbonded forces to group 1 to easily extract their energies and all others to 0 | ||
| for force in hybrid_system.getForces(): | ||
| if force.getName() in ["NonbondedForce", "CustomNonbondedForce", "CustomBondForce_exceptions"]: |
There was a problem hiding this comment.
I think I'm a bit confused about the "CustomBondForce_exceptions", where does that come from?
There was a problem hiding this comment.
CustomBondForce_exceptions is a custom bond force used to interpolate the 1-4 steric and electrostatic exceptions rather than using the nonbonded potential. It allows the use of the softcore potential for sterics as well, so the total nonbonded force in the hybrid system is split across the 3 potentials.
| assert non_zero_exceptions == 9 | ||
|
|
||
| def test_nonbonded_exceptions_dummy(htf_chloro_ethane): | ||
| """Test that the nonbonded exceptions are correctly set when we have dummy atoms, any involving a dummy should be zeroed.""" |
There was a problem hiding this comment.
Do I understand it correctly that we would want to keep exceptions within dummy groups, meaning when all atoms are dummy? If so, should we also have a test with a larger dummy group that checks for that?
There was a problem hiding this comment.
Yeah thats how I assumed this should be done, but I think the HTF is currently interpolating those terms off as well (sudo softening the torsions in the dummy region) I think once we decide how this should be handled then we should add a test for this as well!
Co-authored-by: Hannah Baumann <43765638+hannahbaumann@users.noreply.github.com>
Co-authored-by: Hannah Baumann <43765638+hannahbaumann@users.noreply.github.com>
There was a problem hiding this comment.
From a very brief look through - this seems reasonable. I will have a deeper look through the code when you push these tests again openfe, but please go ahead with the merge here if needed.
3 participants
No description provided.