Remove rigid body data structure from Compound
#1203
Conversation
| @@ -123,10 +123,9 @@ def sixpoints(self): | |||
| return molecule | |||
|
|
|||
| @pytest.fixture | |||
| def rigid_benzene(self): | |||
| def benzene(self): | |||
There was a problem hiding this comment.
I think there's already a benzene in line 27. I would remove that once, as it is from smiles, and keep this mol2 version.
There was a problem hiding this comment.
Removing the benzene from SMILES was causing one of the coarse-graining parmed residue tests to fail (def test_resnames_parmed_cg). I added the fixture back but renamed it so that it's clear it is being loaded from SMILES. I'm not sure why loading it from SMILES was giving different behavior for this test. Might be worth looking at later.
There was a problem hiding this comment.
yeah, this is because RDKIT is what we're using for the from_smiles method, and there is some stochastic nature involved in getting the atomic positions. I think since rdkit is more interested in the chemical connectivity and happy to get a reasonable ground state, but that ground state could be rotated in different ways. Although smiles are really easy to load up for mBuild tests, that one property means that things tend to be less flaky if we just load from a mol2 file.
There was a problem hiding this comment.
Yeah that's true, but unless I'm mistaken, I don't think that specific test (test_resnames_parmed_cg) depends on having consistent atomic positions. It's just creating coarse grain compounds from atomistic compounds.
There was a problem hiding this comment.
Oh so you're saying it's not flaky, it just fails for the mol2 benzene and passes for the smiles benzene? In that case,I think it's the hierarchy, since that can certainly differ based on the information available in those two different formats.
There was a problem hiding this comment.
Yeah, this doesn't seem to be a flaky test, but related to how hierarchy is set.
|
@Zeerakkhan47 I know we discussed having you attempt to make this PR on the last dev meeting, but we felt it was important to get this one done before the 1.0 release, so I wanted to get it done ASAP. Please feel free to review the changes and raise any issues or questions! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1203 +/- ##
==========================================
- Coverage 87.35% 85.73% -1.63%
==========================================
Files 62 53 -9
Lines 6588 4802 -1786
==========================================
- Hits 5755 4117 -1638
+ Misses 833 685 -148 ☔ View full report in Codecov by Sentry. |
PR Summary:
@CalCraven and I have recently been removing the writers from mBuild now that most of them are in GMSO, and during that time some discussion came up about how to handle rigid bodies. Should we keep them in mBuild then read them into GMSO when converting from a compound to a topology, or should all rigid body assignment and handling only be done in GMSO? The only use case for rigid bodies in mBuild were in the HOOMD related writers, and now that they live exclusively in GMSO, we felt it was best to handle rigid body assignment purely in GMSO (See PR #850)
This PR completely removes the rigid body data structure and related methods from
compound.pyand any rigid body assignment done inconversion.pyPR Checklist