nglviewer / ngl

WebGL protein viewer
http://nglviewer.org/ngl/
MIT License
657 stars 168 forks source link

AlphaFold Database CIF files can't be loaded #980

Closed panda-byte closed 1 year ago

panda-byte commented 1 year ago

In my application using ngl@2.0.1, I'm trying to load a structure as follows:

new NGL.Stage()).loadFile(file)

When I try this code while providing file with an upload form, it works for .pdb files from the AlphaFold Database (e.g. https://alphafold.ebi.ac.uk/files/AF-Q5VSL9-F1-model_v4.pdb). However, the same code fails for the CIF download of the same protein (https://alphafold.ebi.ac.uk/files/AF-Q5VSL9-F1-model_v4.cif). The following error occurs:

STAGE LOG error loading file: 'TypeError: Cannot read properties of undefined (reading '0')'

It works, however, for CIF files from PDB, for example. I would assume the CIF files of the AlphaFold DB should be formatted correctly, is that a parsing error by NGL Viewer?

ppillot commented 1 year ago

I think you are right. I can reproduce the error. It seems the parser breaks while expecting the pdbx_PDB_helix_class field: https://github.com/nglviewer/ngl/blob/7701b0294fce4a11c5fc1c1fbb13ef2ea7ceee0b/src/parser/cif-parser.ts#L292

This should be a not too difficult fix.

panda-byte commented 1 year ago

The line in question is part of a loop reading in helix data:

https://github.com/nglviewer/ngl/blob/7701b0294fce4a11c5fc1c1fbb13ef2ea7ceee0b/src/parser/cif-parser.ts#L288-L307

The code relies on the existence of sc.pdbx_PDB_helix_class. Would it work to just replace the condition in the first line?

 if (sc && sc.pdbx_PDB_helix_class !== undefined) {

That would ignore helix data if sc.pdbx_PDB_helix_class is undefined, which should work. I don't know if there's an alternative to using sc.pdbx_PDB_helix_class or if it makes sense to add helix data without, though.

ppillot commented 1 year ago

Yes that's a good approach I think. It's likely that other tests in the same block will break as well. For concision you can use the optional chaining syntax:

var helixType = parseInt(sc?.pdbx_PDB_helix_class)

Which would resolve to parseInt(undefined) ==> NaN I don't have the bandwidth at the moment to investigate this bug, but contributions to the code are welcome!

panda-byte commented 1 year ago

I've created a pull request, the first one I've ever done! Hopefully everything is alright. I've went with if (sc?.pdbx_PDB_helix_class) as a precondition, because it didn't feel right to me to loop through all the values when pdbx_PDB_helix_class is undefined anyway.

Do I need to update the dist directory as well?