-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Parameterized Parmed Conversions #721
Conversation
…nique potential types
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #721 +/- ##
==========================================
+ Coverage 92.05% 92.09% +0.03%
==========================================
Files 66 66
Lines 6370 6350 -20
==========================================
- Hits 5864 5848 -16
+ Misses 506 502 -4
☔ View full report in Codecov by Sentry. |
I tested this with two different numbers of atoms: So I think we're looking pretty much good to merge this. |
Co-authored-by: Co Quach <43968221+daico007@users.noreply.github.com>
Co-authored-by: Co Quach <43968221+daico007@users.noreply.github.com>
Co-authored-by: Co Quach <43968221+daico007@users.noreply.github.com>
The latest commit has addressed the changes to parmed conversion. Required changes due to sorting of GMSO object from ParmEd by topology index:
|
… appearance in the topology. Copies of connection types are made, which must be filtered using potential filters to get whatever subset is considered unique.
@@ -35,9 +35,31 @@ | |||
return potential.member_types or potential.member_classes | |||
|
|||
|
|||
def get_sorted_names(potential): |
Check notice
Code scanning / CodeQL
Explicit returns mixed with implicit (fall through) returns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Only minor comment about an unused variable in a method, but the rest looks great!
gmso/core/views.py
Outdated
"""Get identifier for a topology potential based on name or membertype/class.""" | ||
if isinstance(potential, AtomType): | ||
return potential.name | ||
if isinstance(potential, BondType): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if isinstance(potential, BondType): | |
elif isinstance(potential, BondType): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very minor change for consistency
This PR should fix minor issues #716 which didn't account for symmetries in conversions from parmed structures. This checks that the expected number of potentials are found, which were only being checked over simple molecules such as ethane, and as such missing that some connection types were repeated.
Additional tests were done to check for urey bradleys, harmonic and periodic impropers, and periodic propers.