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

added 'NA' and 'CL' to set {'+', '-'} identifying ions #1086

Merged
merged 2 commits into from
Sep 13, 2018

Conversation

kmboehm
Copy link
Contributor

@kmboehm kmboehm commented Sep 11, 2018

Addressing issue #1026

Yank/yank.py Outdated
'+' in self._topology.atom(i).residue.name]
'+' in self._topology.atom(i).residue.name or
'NA' in self._topology.atom(i).residue.name or
'CL' in self._topology.atom(i).residue.name]
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR! A couple of suggestions:

  1. It looks like you've configured your IDE to use tabs instead of spaces. We shouldn't mix the two types of indentation in Python. Can you switch to 4-space tabs the settings?
  2. Using the in operator here risks to collect things we don't want. What about having maintaining a set of residue names we want to consider ions (beside the ones containing + and -) and have something like
ION_RESIDUE_NAMES = {'NA', 'CL'}
return [i for i in self._solvent_atoms
    if '-' in self._topology.atom(i).residue.name or
    '+' in self._topology.atom(i).residue.name or
    self._topology.atom(i).residue.name in ION_RESIDUE_NAMES]

This way we check for equality instead of looking in the substrings.

Copy link
Contributor

@andrrizzi andrrizzi left a comment

Choose a reason for hiding this comment

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

Looks great!

Copy link
Member

@jchodera jchodera 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!

I was trying to think of whether we could easily expand our test coverage for these cases too, but I'm not sure that would be easy right now, so I'd just go ahead and merge.

@kmboehm kmboehm merged commit 9618299 into choderalab:master Sep 13, 2018
@kmboehm kmboehm deleted the ion-id branch September 13, 2018 16:25
@kmboehm
Copy link
Contributor Author

kmboehm commented Sep 13, 2018

Thanks, guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants