ChainID is overwritten by segid

[lilyminium]

Expected behavior

I can round-trip chainID information from files.

Actual behavior

ChainIDs get overwritten by SegmentIDs, even though the latter is not actually in the original PDB specification. Because the default segment name is "system", this also means that the default chainID is "M" and that's kind of weird.

Code to reproduce the behavior

>>> import MDAnalysis as mda
>>> from MDAnalysis.tests.datafiles import PDB
>>> u = mda.Universe(PDB)
>>> u.atoms.chainIDs = "x"
>>> u.atoms.write("chain_x.pdb")
>>> u = mda.Universe("chain_x.pdb")
>>> u.atoms.chainIDs
array(['M', 'M', 'M', ..., 'M', 'M', 'M'], dtype=object)

And converting it to RDKit and using that to write it out ends up with weird spacing. :(

Current version of MDAnalysis

• Which version are you using? (run python -c "import MDAnalysis as mda; print(mda.__version__)") current develop
• Which version of Python (python -V)?
• Which operating system?

[sidhanthapoddar99]

Hi, @lilyminium can I work on the issue.

[lilyminium]

Please go ahead @sidhanthapoddar99! Put up a pull request and link this issue, and it will get reviewed. If you need help getting started, please check out the user guide, particularly the Contributing section and Contributing code section.

[HenryKobin]

Hi @lilyminium ! I'm taking a look around the codebase regarding this issue, and was curious - Does this affect multiple files or just coordinates/PDB.py, particularly around Line 1083? I guess i'm trying to wrap my head around if this only affects this particular format, or if it's okay to start with trying to fix this one file in particular.

Thanks!

[IAlibay]

@sidhanthapoddar99 are you still looking to work on this issue?

@HenryKobin in the interest of not creating a stressful/competitive environment for prospective GSoC students, it may be best to try to avoid issue fixing collisions if possible. There's no written rule about this, and in the end any help towards our issues is appreciated, but given that there are other issues to work on, it may be best to start with those instead?

Should @sidhanthapoddar99 no longer wish to work on this, or should there be no further progress towards a PR after a reasonable amount of time (maybe a couple more days?), then it would be ok to open a PR instead.

Other @MDAnalysis/gsoc-mentors might have alternate views, and I'm willing to default to their decision here.

[HenryKobin]

Makes sense @IAlibay ! I certainly don't want to cause any stress on this at all. I believe all the other GSoC issues are taken, but I can browse around and see what else is available in the issues list.

[lilyminium]

@HenryKobin maybe give #2604 a go, as the PR for that one stalled some time ago.

[sidhanthapoddar99]

Hi @IAlibay it's fine if @HenryKobin wants to work on this issue, I would require some time in getting around the codebase. When I completely understand the codebase ill try to solve some other issues.

[IAlibay]

I believe all the other GSoC issues are taken,

Ah, this is indeed problematic, we will see what we can do to release/label more GSoC issues.

I do apologise @HenryKobin, we should have implemented better protocols/guidelines so that awkward scenarios like these don't happen. We'll have a chat amongst the mentors to see how we can make this process easier. I am happy to see things did resolve themselves though 😃

Hi @IAlibay it's fine if @HenryKobin wants to work on this issue, I would require some time in getting around the codebase. When I completely understand the codebase ill try to solve some other issues.

Thanks @sidhanthapoddar99 for being understanding !

[HenryKobin]

Thanks @sidhanthapoddar99 ! @IAlibay @lilyminium I'll take a shot at this one and I'll update promptly if I get stalled or can't work on this issue.

[lilyminium]

@HenryKobin I would start with trying to fix PDB.py to start off with. All the formats have the segids that line 1083 gets, but not all file formats have chainIDs. So far as I can tell, the other formats listed there (e.g. XPDB) don't have writer classes associated, so roundtripping is not yet a problem there :-)

[HenryKobin]

@lilyminium cool! I'm just about done with the fix, and I'm looking over the test suite right now. There appears to be a test that asserts that the chainID will come from the last character of the segid. I'm guessing I can remove this test and replace it with a test that is similar to the "reproduce this behavior" section except it passes?

I'll be opening up a PR soon, so i'll just leave that up to the team to review but i'm going to go ahead and replace the test for now.

[lilyminium]

@HenryKobin yes, do put up the PR. However, I think it is expected that when a chainID does not exist (vs. being empty), then it comes from the last character of the segID. In this issue I set the chainID to "x" and it was still getting written out as "M", which is the case that I would like to fix.

Thanks for working on this!