Skip to content
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

Merged
merged 13 commits into from
May 17, 2023

Conversation

CalCraven
Copy link
Contributor

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.

@CalCraven CalCraven self-assigned this Apr 26, 2023
@CalCraven CalCraven linked an issue Apr 26, 2023 that may be closed by this pull request
@CalCraven CalCraven requested a review from daico007 April 26, 2023 23:38
gmso/external/convert_parmed.py Fixed Show fixed Hide fixed
gmso/external/convert_parmed.py Fixed Show fixed Hide fixed
@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

Patch coverage: 94.68% and project coverage change: +0.03 🎉

Comparison is base (78b0d85) 92.05% compared to head (3367c3a) 92.09%.

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     
Impacted Files Coverage Δ
gmso/external/convert_parmed.py 95.47% <93.50%> (-0.21%) ⬇️
gmso/core/views.py 95.34% <100.00%> (+1.06%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@CalCraven
Copy link
Contributor Author

CalCraven commented Apr 27, 2023

I tested this with two different numbers of atoms:
18000 atoms -> 2.24 +- 0.09 s
45000 atoms -> 5.80 +- 0.21 s

So I think we're looking pretty much good to merge this.

gmso/external/convert_parmed.py Outdated Show resolved Hide resolved
gmso/external/convert_parmed.py Outdated Show resolved Hide resolved
gmso/external/convert_parmed.py Outdated Show resolved Hide resolved
gmso/external/convert_parmed.py Outdated Show resolved Hide resolved
gmso/external/convert_parmed.py Outdated Show resolved Hide resolved
gmso/external/convert_parmed.py Outdated Show resolved Hide resolved
CalCraven and others added 3 commits May 1, 2023 12:12
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>
@CalCraven
Copy link
Contributor Author

The latest commit has addressed the changes to parmed conversion. Required changes due to sorting of GMSO object from ParmEd by topology index:

  • Tests using parmed objects have different ordering
  • New potential filter UNIQUE_SORTED_NAMES
  • Comparison of unique types using different potential filters
  • ParmEd back and forth conversions will result in some connections that are flipped

… 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

Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
Copy link
Member

@daico007 daico007 left a 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!

"""Get identifier for a topology potential based on name or membertype/class."""
if isinstance(potential, AtomType):
return potential.name
if isinstance(potential, BondType):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if isinstance(potential, BondType):
elif isinstance(potential, BondType):

Copy link
Member

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

gmso/external/convert_parmed.py Outdated Show resolved Hide resolved
gmso/external/convert_parmed.py Outdated Show resolved Hide resolved
gmso/external/convert_parmed.py Outdated Show resolved Hide resolved
gmso/external/convert_parmed.py Outdated Show resolved Hide resolved
gmso/external/convert_parmed.py Outdated Show resolved Hide resolved
gmso/external/convert_parmed.py Outdated Show resolved Hide resolved
gmso/external/convert_parmed.py Outdated Show resolved Hide resolved
@daico007 daico007 merged commit d068b93 into mosdef-hub:main May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue in conversion from parmed
2 participants