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

Update connection members during parameterization #808

Merged
merged 1 commit into from
Mar 17, 2024

Conversation

CalCraven
Copy link
Contributor

Add tests and change behavior when applying types, update connection connection_members from types.

This addressed a bug found by @bc118 using NAMD, which write the indices of impropers which must match with the types of the improper types listed.

image This screenshot identifies the issues most clearly with impropers. Order is only enforced from identifying the improper in the forcefield. As such, we have to do reordering once the improper_type is identified.

This is a potentially impactful issue if using improper in GMSO. Anyone doing so should check that the improper atoms in their output files are ordered as expected, with the central atom first, and the last atom last.

Comment on lines +69 to +73
# for connection in getattr(top2, connection_type):
# connection_types_mirror[
# tuple(
# top2.get_index(member)
# for member in sort_connection_members(connection, "atom_type")

Check notice

Code scanning / CodeQL

Commented-out code Note test

This comment appears to contain commented-out code.
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to remove these commented out block?

Comment on lines +77 to +81
# for connection in getattr(top1, connection_type):
# connection_types_original[
# tuple(
# top1.get_index(member)
# for member in sort_connection_members(connection, "atom_type")

Check notice

Code scanning / CodeQL

Commented-out code Note test

This comment appears to contain commented-out code.
Copy link

codecov bot commented Mar 11, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 92.45%. Comparing base (69fd5e2) to head (0a8b944).

Files Patch % Lines
gmso/utils/sorting.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #808      +/-   ##
==========================================
+ Coverage   92.44%   92.45%   +0.01%     
==========================================
  Files          66       66              
  Lines        7024     7036      +12     
==========================================
+ Hits         6493     6505      +12     
  Misses        531      531              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@bc118 bc118 left a comment

Choose a reason for hiding this comment

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

LGTM. I tested it on the problematic system, and it seems to run on MoSDeF-GOMC test also.

@CalCraven CalCraven requested a review from daico007 March 13, 2024 21:37
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 stuff regarding docs and styles

Comment on lines +69 to +73
# for connection in getattr(top2, connection_type):
# connection_types_mirror[
# tuple(
# top2.get_index(member)
# for member in sort_connection_members(connection, "atom_type")
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to remove these commented out block?

Comment on lines 44 to 45
The attribute of the site to sort by. Can take "name", "atom_type",
and "atom_class" as the sorting attribute.
Copy link
Member

Choose a reason for hiding this comment

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

Need to add "index" here (with contignent of top is not None)

@CalCraven CalCraven merged commit d3fca73 into mosdef-hub:main Mar 17, 2024
14 checks passed
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.

3 participants