-
Notifications
You must be signed in to change notification settings - Fork 5
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
Feat/parameterizer interface #22
base: main
Are you sure you want to change the base?
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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.
Overall, this is looking good! Well done.
A couple things I'd like to see before merging:
-
tests passing
-
it'd be awesome to get a line-by-line walkthrough of the parameterizer.py during a synchronous meeting.
def UNEXPECTED_TOPOLOGY_TYPE(parametizer_type, actual, expected): | ||
return f"{parametizer_type.name} requires topology of type {expected} but got type {actual}" |
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.
Really nice messaging approach!
INTERCHANGE_PARAMETERIZER: dict[str,list[str]] = { | ||
"sage": ["openff_unconstrained-2.0.0.offxml"] | ||
} | ||
DEFAULT_PARAMETERIZER: dict[str,list[str]] = OPENMM_PARAMETERIZER |
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.
Let's make the default Interchange
from typing import List, Optional, Dict | ||
|
||
|
||
class TestParameterizer(unittest.TestCase): |
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.
Are tests currently passing?
}, | ||
{ | ||
"smile":"[Cl-]", | ||
"force_field": "amoeba", |
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.
What's the behavior if no force_field is specified?
It'd be great to have an option to specify a default FF for all mol_specs
If custom force field files are being used with a non-OpenMM parameterizer. | ||
If no custom force field files are provided when using custom force fields. |
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.
How are we currently supporting arbitrary .xml files that aren't in the interchange, openmm, or openff lists?
creating a new pull request bc my fork got messed up so this was easier to do. Still have some final cleanup to do but the functionality is all there.