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.

Sign up for GitHub

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

Jump to bottom

#999 cif parsing #1001

Merged
merged 2 commits into from
Nov 20, 2023
Merged

#999 cif parsing #1001

merged 2 commits into from
Nov 20, 2023

Conversation

ppillot
Copy link
Collaborator

@ppillot ppillot commented Nov 19, 2023

This PR fixes 2 bugs in relation with CIF files parsing:

  • NGL error displaying some mmCIF files #999 chem_comp can be set for macromolecular cif files and contain vectors of entities. In the case of mmCIF files the block of code that parses files containing only one entity (such as PDB het cif files) must not be reached.
  • NGL shows white color for this cif #950 core cif files often define charges as part of the atom type symbol. In that case, the element symbol must be stripped of the charge.

@ppillot
nglviewer#999 _chem_comp can be defined on mmCIF files
82950b4 
The code was assuming that the _chem_comp property was only available
for non macromolecular structures (such as cif
files for hetero atoms entities).
It appears that some PDB files (e.g. `32C2`) may define
a _chem_comp loop around every
chemical component such as its amino-acids.
This breaks the previous code as `cif.chem_comp` then contains
an array of strings instead of a string.
The fix here is to assume that if `struct` is present then we should not
enter in the block for parsing simple entities.
@ppillot
nglviewer#950 handle type symbols with charges
bbd7a7e
@ppillot ppillot requested review from fredludlow and hainm November 19, 2023 23:54
@hainm
Copy link
Contributor

hainm commented Nov 20, 2023

@ppillot are you able to reproduce the bug without your change and see if the change actually fixes the issue?

@hainm
Copy link
Contributor

hainm commented Nov 20, 2023

Also, do you have any idea why nglview fails to show 3pqr recently, althought it has been working well for years (because we have been showing it as example in our nglview website)

image

Here is the code in nglview: https://github.com/nglviewer/nglview/blob/d9e7740100b586b1e73cc51d247d16b6dc8d0914/nglview/adaptor.py#L123-L125

fredludlow
fredludlow approved these changes Nov 20, 2023
View reviewed changes
Copy link
Collaborator

@fredludlow fredludlow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Tested locally. Both the 3pqr.cif file (downloaded from http://www.rcsb.org/pdb/files/3pqr.cif) and the one posted under #950 were previously reproducibly broken, but are now working with this patch.

@fredludlow fredludlow merged commit 7d271db into nglviewer:master Nov 20, 2023
2 checks passed
@hainm
Copy link
Contributor

hainm commented Nov 20, 2023

Thank you. Hopefully there will be a release soon.

@ppillot
Copy link
Collaborator Author

ppillot commented Nov 21, 2023

@hainm release done and available on npm

@hainm
Copy link
Contributor

hainm commented Nov 21, 2023 via email

@padiauj padiauj mentioned this pull request Dec 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fredludlow fredludlow fredludlow approved these changes

@hainm hainm Awaiting requested review from hainm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ppillot @hainm @fredludlow