Add get_associations method to topology class (Closes #375) #378
Conversation
adds get_association method to return a set of sites associated with an atomtype or a set of bonds associated with a bond_type and so on.
Codecov Report
@@ Coverage Diff @@
## master #378 +/- ##
==========================================
+ Coverage 93.09% 93.12% +0.02%
==========================================
Files 74 74
Lines 5084 5133 +49
==========================================
+ Hits 4733 4780 +47
- Misses 351 353 +2
Continue to review full report at Codecov.
|
| typed_ar_system.atom_types[0].name = 'Typed Ar System' | ||
| assert typed_ar_system.get_associations(typed_ar_system.atom_types[0]) == set(typed_ar_system.sites) | ||
|
|
||
| def test_get_association_bond_types(self, typed_water_system): |
There was a problem hiding this comment.
not sure if this fits here, but what if we had a system with bonds (water would be an example: H-O-H)
In this case, bondtype should have a set that contains all 3 sites. If we then removed a bond between one of the H and O (H O-H)
We expect that the len(get_associations(broken_water) is now 2.
Would be a good test to have. and this same example should work for angletype too.
There was a problem hiding this comment.
Look good! Thanks @umesh-timalsina, I just had two minor comments. We also discussed Monday changing get_associations to get_type_associations. I personally like this change but would like to get the opinions of @mosdef-hub/mosdef-contributors as well.
| self._connection_types = {} | ||
| self._bond_types = {} | ||
| self._bond_types_associations = {} |
There was a problem hiding this comment.
Really small nit but types is plural in _atom_types_associations and _bond_types_associations but is singular in _angle_type_associations. I think I would actually prefer these all be singular type (_atom_type_associations, _bond_type_associations, etc.) because it reads a little bit better to me, but it's worth getting opinions from the rest of @mosdef-hub/mosdef-contributors.
Either way the naming should be consistent.
There was a problem hiding this comment.
The reason for naming it with _****_types_associations was because it is a dictionary of all the connection types in the topology, which will contain a set. I have no preference one way or other.
There was a problem hiding this comment.
Got it, I'm fine with _****_types_associations then.
|
Probably better to close it and open it again. |
adds
get_associationmethod to return a set of sites associated with anatomtypeora set of bonds associated with a bond_type and so on.