-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add supported charge methods to API #1834
Conversation
Codecov Report
Additional details and impacted files |
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 generally like the structure and goals of this PR. Only blocking point is whether this needs to be an API break. It's possible I'm overlooking something but let's discuss on that review comment.
@@ -37,6 +37,26 @@ class AmberToolsToolkitWrapper(base_wrapper.ToolkitWrapper): | |||
"The AmberTools toolkit (free and open source) can be found at " | |||
"https://anaconda.org/conda-forge/ambertools" | |||
) | |||
_supported_charge_methods = { |
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.
(blocking) Would it be possible to leave this as SUPPORTED_CHARGE_METHODS
to avoid an API change?
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 can maintain the API at the cost of duplicated code
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've made an attempt to do that - unfortunately the wrappers disagree on what to call this attribute, so it's not easy to move it into the base class
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.
Awesome, thanks @mattwthompson! Please update the releasenotes before you merge 🚀
@@ -9,6 +9,7 @@ Releases follow the `major.minor.micro` scheme recommended by [PEP440](https://w | |||
## Current development | |||
|
|||
* #1798 Adds type annotations to most of the codebase. | |||
* #1834 Adds `Molecule.get_available_charge_methods` and `BaseWrapper.supported_charge_methods`. |
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.
@j-wags just noting here that these have different names; I think there's okay reason for it
Implements #1830