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

Bugfixed chain ID assignment in addSolvent (#287) #294

Merged
merged 3 commits into from
Jun 26, 2024

Conversation

murfalo
Copy link
Contributor

@murfalo murfalo commented Jun 21, 2024

How does this approach work for fixing #287? This should assign B-Z if the previous chain ID was A-Y, and otherwise leave the ID as a numeric chain ID.

One comment: I noticed PDBFile and PDBxFile simply use % 26 to map numeric IDs to the alphabet. Should we warn the user somehow that disparate chains may be assigned matching IDs in these cases?

@peastman
Copy link
Member

I think we can do something much simpler. Remember, writeFile() will be assigning new chain IDs anyway. So it doesn't really matter much what we assign here. We could do something like this.

lastid = chains[-2].id
if len(lastid) == 1 and 'A' <= lastid < 'Z':
    chains[-1].id = chr(ord(lastid)+1)
else:
    chains[-1].id = 'A'

We need to make this change both in addSolvent() and addMembrane().

@murfalo
Copy link
Contributor Author

murfalo commented Jun 22, 2024

Thanks for the feedback, particularly the much simpler if case and ID assignment. I've implemented that and also taken out the assertions, .upper(), and verbose commentary to better match existing code style.

Before porting code over to addMembrane(), I have two questions regarding your proposed method:

  1. My understanding is that addSolvent will typically add two chains (solvent and ion) and addMembrane will typically add three chains (lipid, solvent, and ion). In these cases, couldn't multiple assignments be required, and wouldn't lastid = chains[-2].id most often refer to the numeric ID of the newly created solvent chain?
  2. In either case, do you think it's worth accounting for an edge case where the user specifies 0 ionic strength?

@peastman
Copy link
Member

Good point. The number of chains added could be either 1 or 2 for addSolvent(), and either 2 or 3 for addMembrane(). Maybe write a _renameNewChains() method that can be called in both cases? You'll pass in the number of chains that previously existed, and it will figure out how many new ones were added and pick appropriate names for them.

@murfalo
Copy link
Contributor Author

murfalo commented Jun 26, 2024

Done! Added _renameNewChains() and updated addSolvent()and addMembrane(). How do things look?

@peastman
Copy link
Member

Looks good. Thanks!

@peastman peastman merged commit 268f1d7 into openmm:master Jun 26, 2024
3 checks passed
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.

2 participants