[WIP] Fix OpenMM Constraints #969
Conversation
|
@swails I made a fix for the OpenMM Constraints in Parmed and all the tests are passed. The main goal of the patch was to fix the Parmed rigid water option that was not behaving correctly leaving the water partially flexible. In the patch, I still have some concerns related to cases where angle constraints could involve for example virtual sites. I wonder if you can comment on that and overall if you could have a look at the patch and have it merged. Thanks |
parmed/structure.py
Outdated
| constraint_angle_set = set() | ||
| is_water = _settler(self) | ||
|
|
||
| if constraints == app.AllBonds or constraints == app.HAngles: |
There was a problem hiding this comment.
app.AllBonds and app.HAngles are better thought of as enums (or singletons). Compare using is, not ==
parmed/structure.py
Outdated
| if isinstance(bond.atom1, ExtraPoint): continue | ||
| if isinstance(bond.atom2, ExtraPoint): continue | ||
| constraint_bond_set.add(frozenset((bond.atom1.idx, bond.atom2.idx))) | ||
| elif constraints == app.HBonds: |
parmed/structure.py
Outdated
|
|
||
| if constraints == app.HAngles: |
parmed/structure.py
Outdated
| if rigidWater: | ||
| for angle in self.angles: | ||
| if is_water[angle.atom1.residue.idx]: | ||
| constraint_angle_set.add(frozenset((angle.atom1.idx, |
There was a problem hiding this comment.
Order matters here, doesn't it? So you can't use an unordered collection like set or frozenset. My guess is that it will usually work, but there may be instances where it'll fail.
|
Thanks! Looks good! My one last comment is that the angle ordering now won't match the equivalent, but reversed ordering. However, since this one method constructs the set of constraints to add, and then adds them, from the same I'll merge this as-is. |
|
Thank you so much, |
|
@swails I'm sorry to bother you again, we would like to make a new conda package in the omnia repository. Would be possible for you to update the Parmed point release to 2.7.4. Thanks a lot |
|
To be specific, we're using this on a cloud computing platform where (for practical purposes) we can't use software that isn't available as a conda-installable point release, and having a bug where we accidentally end up with semi-flexible water is obviously not a good way for us to successfully run our calculations. So we can't proceed until we have a new point release (we can update the conda recipe to reflect the new release). |
|
Done and tagged. I think all that needs to be done is someone has to update the conda-recipes repo where the recipe for ParmEd lives. |
|
It's now pip-installable as version 3.0.1 as well, which should help workaround any delays in conda-availability |
This is a fix for the issues: #958 and #959