OpenMMEngine: use masses from mdtraj topolgy directly#1085
Conversation
…therfore forcefield) are not know
Codecov Report
@@ Coverage Diff @@
## master #1085 +/- ##
=======================================
Coverage 81.56% 81.56%
=======================================
Files 140 140
Lines 15451 15451
=======================================
Hits 12603 12603
Misses 2848 2848
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Could you add a small regression test? It looks like all you'd need is a PDB with a single TIP4P water from your example. Put that in tests/test_data/, load it up (use the tests.test_helpers.data_filename method to get the file location regardless of install method), and then pass it through this process. Test would go in tests/test_openmm_snapshot.py.
Otherwise, LGTM! Thanks!
dwhswenson
left a comment
There was a problem hiding this comment.
Test looks good to me. Any other changes you're planning in this, @hejung, or is it ready to go?
|
@dwhswenson All ready to go. Yesterday evening one if the tests failed in its setup phase and I had planed to check again today in the hope it would resolve itself, but you were faster than me :) |
|
Yeah, it looked like network connectivity was a little rough yesterday -- It took a couple of reruns to get it to work, but it's easy enough for me to just click the button! Merging now; thanks for the bugfix! |
This avoids the error raised by the fact that virtual sites have no element associated with them in openMM by taking the mdtraj topology directly. From our discussion in #1084 I gathered that using the mdtraj masses directly would be an acceptable solution?
This avoids the situation in which we have to change the engine of the snapshots to be able to use the
RandomVelocitiesmodifier with virtual sites as in TIP4P. One should still pass asubset_maskin which the virtual sites are excluded and the OpenMMEngine asengineto be able to apply constraints to theRandomVelocitiesmodifier on creation.