-
Notifications
You must be signed in to change notification settings - Fork 37
Bonds and Angles handle wildcards for bonds and sites. #928
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
Conversation
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #928 +/- ##
==========================================
- Coverage 94.20% 93.93% -0.27%
==========================================
Files 67 67
Lines 7591 7668 +77
==========================================
+ Hits 7151 7203 +52
- Misses 440 465 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
I left some comments, mostly related to doc strings. So far this is looking great. I'll try and get some minimal working examples going to further test it out.
|
|
||
| __members_creator__: ClassVar[Callable] = Atom.model_validate | ||
|
|
||
| connectivity: ClassVar[Tuple[Tuple[int]]] = ((0, 1), (1, 2)) |
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.
We should add a description here.
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.
Also, do we need an alias?
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.
I specified connectivity as a ClassVar, so it's not modifiable across classes, and doesn't get treated the same way as other pydantic variables, which are defined under with pydantic Field.
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.
So also no alias here.
|
|
||
| __members_creator__: ClassVar[Callable] = Atom.model_validate | ||
|
|
||
| connectivity: ClassVar[Tuple[Tuple[int]]] = ((0, 1),) |
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.
Same as in angle.py. Do we need a description and alias here?
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.
See above.
gmso/core/forcefield.py
Outdated
| raise MissingPotentialError(msg) | ||
|
|
||
| def _get_angle_type(self, atom_types, return_match_order=False, warn=False): | ||
| for i in range(1, 3): |
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.
TODO: This masking schema won't work, should try masking outside in, i.e. -opls_135- should be masked before opls_135--.
|
This PR will not work until mosdef-hub/foyer#640 is completed. |
…gmso into wildcard-apply
…to wildcard-apply
for more information, see https://pre-commit.ci
… and dynamically set bonds based on connection_members when initialized with set_dependent_value_default
…via masking, remove unused functions
PR Summary:
This PR should allow for matching of forcefield connection parameters for Bonds and Angles. This was already usable with Dihedrals, the functionality has just been extended to dihedrals using the "*" syntax.
Additionally, the ability to match connections by the bond_order of the internal bonds of the connection is also included, using the "~" syntax to designate wildcard bonds, and the SMILES tokens for single, double, triple, and aromatic bonds.
PR Checklist