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

Add option to control whether water is alchemically changed into counterion for charge changes #1030

Merged
merged 11 commits into from
Jun 17, 2022

Conversation

zhang-ivy
Copy link
Contributor

@zhang-ivy zhang-ivy commented Jun 1, 2022

Description

This PR allows the user to disable the introduction of counterion(s) for small molecule and protein mutation transformations.

Motivation and context

Resolves #1004

How has this been tested?

@ijpulidos : Could you make sure that specifying transform_waters_into_ions_for_charge_changes : False in the yaml actually controls the introduction of the counterion? If the changes I made are right, you should see a logger message that says Skipping counterion.

Note that I added a test (for protein mutations, but not small molecules) that checks that this option is working properly. You could adapt this test for a small molecule example, or it might be sufficient to just double check that you see the logger statement when you include the transform_waters_into_ions_for_charge_changes option in the yaml.

Change log

Add option for disabling the introduction of a counterion for charge changing transformations

@zhang-ivy
Copy link
Contributor Author

zhang-ivy commented Jun 1, 2022

Note that this PR should not be merged until #1024 has been merged, as I made some changes to the protein mutation API. Those changes will need to be carefully merged with these ones -- I can take care of that once #1024 is merged.

@codecov
Copy link

codecov bot commented Jun 1, 2022

Codecov Report

Attention: Patch coverage is 91.30435% with 2 lines in your changes missing coverage. Please review.

Project coverage is 52.32%. Comparing base (a2cdad2) to head (fa0111a).
Report is 65 commits behind head on main.

Additional details and impacted files

Copy link
Contributor

@ijpulidos ijpulidos left a comment

Choose a reason for hiding this comment

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

Looks good, great work. Just a couple of things:

  • We need to use the option read from the YAML file.
  • Do we want a test that directly computes the charges?

@ijpulidos ijpulidos self-requested a review June 17, 2022 17:55
Copy link
Contributor

@ijpulidos ijpulidos left a comment

Choose a reason for hiding this comment

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

LGTM!

@ijpulidos
Copy link
Contributor

ijpulidos commented Jun 17, 2022

By the way, just confirmed locally that the YAML file option is working correctly.

@ijpulidos
Copy link
Contributor

@zhang-ivy Could you please add what would need to appear in the changelog for these changes? Thanks!

@zhang-ivy
Copy link
Contributor Author

@ijpulidos : Done!

@ijpulidos ijpulidos merged commit 182bf16 into main Jun 17, 2022
@ijpulidos ijpulidos deleted the control-counterion branch June 17, 2022 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high priority high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to control whether water is alchemically changed into counterion for charge changes
4 participants