openmm / pdbfixer

PDBFixer fixes problems in PDB files
Other
443 stars 112 forks source link

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

Closed murfalo closed 2 weeks ago

murfalo commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

peastman commented 2 weeks ago

Looks good. Thanks!