-
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
Update connection members during parameterization #808
Conversation
…connection_members from types
# 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
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.
Do we want to remove these commented out block?
# 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
Codecov ReportAttention: Patch coverage is
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. |
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. I tested it on the problematic system, and it seems to run on MoSDeF-GOMC test also.
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 stuff regarding docs and styles
# for connection in getattr(top2, connection_type): | ||
# connection_types_mirror[ | ||
# tuple( | ||
# top2.get_index(member) | ||
# for member in sort_connection_members(connection, "atom_type") |
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.
Do we want to remove these commented out block?
The attribute of the site to sort by. Can take "name", "atom_type", | ||
and "atom_class" as the sorting attribute. |
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.
Need to add "index" here (with contignent of top is not None)
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.
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.