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.

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

fix: allow ccd residues with missing coords #730

Merged
merged 6 commits into from
Jan 8, 2025

Conversation

davegrays
Copy link
Contributor

@davegrays davegrays commented Dec 27, 2024

There are a number of ligands/residues in the CCD with missing coordinates that cannot be retrieved now with structure.info.residue due to a ValueError that is raised twice and not excepted the second time. Relative to v0, this is new behavior.

This allows these residues to be returned from the second attempt with nans where missing.

Provides an exception so that residues with missing coords in the ccd can still be returned from the ideal coordinates.
@davegrays davegrays changed the title Allow ccd residues with missing coords fix: allow ccd residues with missing ideal coords Dec 27, 2024
@davegrays davegrays changed the title fix: allow ccd residues with missing ideal coords fix: allow ccd residues with missing coords Dec 27, 2024
Copy link

codspeed-hq bot commented Dec 27, 2024

CodSpeed Performance Report

Merging #730 will not alter performance

Comparing davegrays:fix/allow-all-ccd-residues (4e931fb) with main (226f2b4)

Summary

✅ 59 untouched benchmarks

@padix-key
Copy link
Member

Hi, from my perspective having a molecule without atom coordinates has barely a benefit. Consequently, getting an exception, that can be easily captured, is more clearer than checking for NaN coordinates in my opinion. However, I would be curious what your use case is.

@davegrays
Copy link
Contributor Author

@padix-key Makes sense. My use case is that I'm identifying missing atoms in the experimental structure by constructing molecular templates from the CCD (essentially just the list of atom names per residue). So for that I use the CCD's AtomArray and I'm ignoring the coords anyway.

I get your concern tho... what would you think about an optional arg for this in get_components() that can propagate to structure.info.residue?

@Croydon-Brixton
Copy link
Contributor

I agree with @davegrays that optionally exposing missing atoms in the builtin CCD is useful for building template residues of 'what should be there if all atoms were resolved' and thereby allows checking for missing atoms.

I like the idea of exposing a flag in get_components for this -- here 's a patch MR for this davegrays#2

feat(pdbx): expose flag to allow missing coorindates in `get_component`
@davegrays
Copy link
Contributor Author

davegrays commented Jan 2, 2025

Thanks @Croydon-Brixton ! Merged your patch

@padix-key I implemented the remaining fix to propagate the allow_missing_coords flag to structure.info.residue. Curious what you think now.

Copy link
Member

@padix-key padix-key left a comment

Choose a reason for hiding this comment

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

Hi, sorry for the delay. I see the point now 👍.

The PR looks mostly good to me, what misses is only a test in tests/structure/test_info() where residue() loads a molecule with missing coordinates.
Apart from that I added some small code style suggestions to the review.

src/biotite/structure/info/atoms.py Outdated Show resolved Hide resolved
src/biotite/structure/info/atoms.py Outdated Show resolved Hide resolved
src/biotite/structure/info/atoms.py Outdated Show resolved Hide resolved
src/biotite/structure/io/pdbx/convert.py Outdated Show resolved Hide resolved
src/biotite/structure/io/pdbx/convert.py Outdated Show resolved Hide resolved
src/biotite/structure/io/pdbx/convert.py Outdated Show resolved Hide resolved
@davegrays
Copy link
Contributor Author

@padix-key thanks for your feedback! all the style changes are implemented and I've added test cases for a few different molecules

@davegrays davegrays requested a review from padix-key January 7, 2025 01:36
@davegrays
Copy link
Contributor Author

davegrays commented Jan 7, 2025

also not sure exactly why but it seems like the ci/cd step that failed on this last commit was an intermittent network-related issue. i only changed the test itself and i can confirm the test still passes

@padix-key
Copy link
Member

I just run it again, and check if it passes. But I agree, this should have nothing to do with your changes

@padix-key
Copy link
Member

The CI now passes and everything looks good to me. Thank you!

@padix-key padix-key merged commit 8138e0e into biotite-dev:main Jan 8, 2025
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants