Implementing gemmi-based mmcif reader (with easy extension to PDB/PDBx and mmJSON) - #4712
Implementing gemmi-based mmcif reader (with easy extension to PDB/PDBx and mmJSON)#4712marinegor wants to merge 145 commits into
gemmi-based mmcif reader (with easy extension to PDB/PDBx and mmJSON)#4712Conversation
|
Hello @marinegor! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-10-25 11:17:29 UTC |
Linter Bot Results:Hi @marinegor! Thanks for making this PR. We linted your code and found the following: Some issues were found with the formatting of your code.
Please have a look at the Please note: The |
richardjgowers
left a comment
richardjgowers
left a comment
There was a problem hiding this comment.
Looks good so far, will require a small test file to check reader/parser halves.
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
I only have minor doc fix/change requests. Looks good to go from the technical side. There are some explicit python loops that look slow but as the comments say, check back when the gemmi Python API gives access to flat structures.
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
Thanks for addressing my comments; all good from my side.
(I left optional suggestions, which are really flavor-to-taste, whatever you think works better.)
Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
|
@IAlibay If we are are headed for a new release, would love to get this PR merged & part of if if you could take a final look. |
|
+1 on seeing this merged into the next release! |
|
@IAlibay your comment had been addressed and the gemmi reader is also tested in Windows on azure. Could you updated your review, please? Otherwise I'll dismiss it in a day in an effort to merge this important new feature. (I am not sure what the linter wants, I'll try to make it happy.) |
@orbeckst can you give me until Tuesday please? I agree it's an important feature and I do want to review it, however there's some other high priority items within MDAnalysis that needs addressing first. Either way, I'm not going to be releasing a be version of MDAnalysis until after the 17th - so it not being merged until then isn't an issue. |
|
Ok |
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Sorry for the very brief review, mostly cleaning things to do.
| name = "MDAnalysis" | ||
| dynamic = ['version', 'readme'] | ||
| license = {file = "LICENSE"} | ||
| license = { file = "LICENSE" } |
| description = "An object-oriented toolkit to analyze molecular dynamics trajectories." | ||
| authors = [ | ||
| {name = 'MDAnalysis Development Team', email = 'mdanalysis@numfocus.org'} | ||
| { name = 'MDAnalysis Development Team', email = 'mdanalysis@numfocus.org' }, |
There was a problem hiding this comment.
same here and everywhere else in this file, please revert the extra spaces
There was a problem hiding this comment.
I see this one is gzipped, is there a reason why the non gzipped version is being kept around?
There was a problem hiding this comment.
Some of the coordinate and topology tests are parameterized with gzipped and non-gzipped versions of the CIF files to ensure both can be parsed (zipping/unzipping could be done at test-runtime though if disk usage is the concern)
There was a problem hiding this comment.
Zipping/unzipping should be being handled by the anyopen mechanism, it's unclear to me why this needs separate testing. However, my request here would be to limit it to a single file, the rest should be gzipped.
There was a problem hiding this comment.
Also needs an entry for mmcif in this file.
Documentation build overview
99 files changed ·
|
|
Atoms without altLocs in the MMCIF will have null bytes assigned to their altLoc attribute. When writing the structure out to PDB, it includes those null bytes, in violation of the PDB spec (not that this have stopped anyone before). Not fully sure of the consequences but figured I'd raise it before it's merged. Altered tail of 1BD2.cif to include non-null altLocs: Tail of reconstructed PDB (but \0 is actually a null byte) You can recreate this with the following: https://gist.github.com/ianmkenney/624300e0395bcf2c918c2fa8b92fd86e Just clone MDA and check out the PR head next to the Makefile and run |
| "Found an atom that is neither ATOM nor HETATM" | ||
| ) | ||
| for atom in residue: | ||
| altlocs.append(atom.altloc or "A") |
There was a problem hiding this comment.
As stated in https://gemmi.readthedocs.io/en/latest/mol.html:
altloc is stored as a single character. Majority of atoms has a single conformations and the altloc character set to NUL ('\0'). If you want to check if an atom has non-NUL altloc, you may also use method has_altloc()
| altlocs.append(atom.altloc or "A") | |
| altlocs.append(atom.altloc if atom.has_altloc() else "") |
I think this is consistent with how we want to handle this given the MMTFParser's code:
attrs.append(
AltLocs(
[
val.replace("\x00", "").strip()
for val in mtop.alt_loc_list
]
)
)
11 participants
Fixes #2367 and also extends #4303 and solves #5089
Changes made in this Pull Request:
gemmilibrary (link) to parse mmcif filesclass MMCIFReader(base.SingleFrameReaderBase)andclass MMCIFParser(TopologyReaderBase)classes for thatAs a bonus, this implementation would potentially allow to read any of the gemmi-supported formats (source):
Also, this (with slight modifications) also would allow reading mmcif with multiple models sharing the same topology, as well as more feature-rich parsing of PDBs (the same code without changes can be used for parsing altlocs, charges, etc, from all of these formats).
However, I'm slightly lost on what's to be done next for this PR to be merged, so I'm asking if someone could help me navigate here (tagging @richardjgowers here as author of original PDBx implementation 4303).
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4712.org.readthedocs.build/en/4712/