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

Feat/update vdw radii #745

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Croydon-Brixton
Copy link
Contributor

I had a detailed look into the latest open access vdW radii, especially for elements that were not previously represented in biotite. I updated the values with recent authoritative sources, which should now allow SASA computations for almost all structures. I also did a comparison against non-open access reference values which we cannot use here, but made sure that open access values we use are close to (usually <0.05A) those reference values.

Copy link

codspeed-hq bot commented Jan 29, 2025

CodSpeed Performance Report

Merging #745 will not alter performance

Comparing Croydon-Brixton:feat/update_vdw_radii (c43a0a5) with main (d979def)

Summary

✅ 59 untouched benchmarks

@Croydon-Brixton
Copy link
Contributor Author

Not sure what's happening on the CI side here as I only changed a dictionary -- maybe a glitch?

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.

Thanks for the update. I only left two minor comments, otherwise this looks good!

I will also add the new reference (Mantina2009) to the Biotite Zotero library.

Regarding the failing test: It fails, because the reference SASA values were computed with mdtraj using the old radii. To update the reference values to the new radii, just rerun sasa.py (requires mdtraj). I can support you here, if you need some help.

@@ -83,7 +83,8 @@ def sasa(array, float probe_radius=1.4, np.ndarray atom_filter=None,
- **Single** - A set, which uses a defined VdW radius for
every single atom, therefore hydrogen atoms are required
in the model (e.g. NMR elucidated structures).
:footcite:`Bondi1964`
Values for main group elements are taken from :footcite:`Mantina2009`,
and for relevant transition metals from the RDKit.
Copy link
Member

Choose a reason for hiding this comment

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

Could you also add a citation to the relevant RDKit paper here?

Suggested change
and for relevant transition metals from the RDKit.
and for relevant transition metals from *RDKit*.

("I", 1, 0) : 1.98, # Taken from _SINGLE_RADII
}

_SINGLE_RADII = {
"H": 1.20,
_SINGLE_ELEMENT_VDW_RADII = {
Copy link
Member

Choose a reason for hiding this comment

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

Just an optional nit: I think single atom is more appropriate than single element here.

Suggested change
_SINGLE_ELEMENT_VDW_RADII = {
_SINGLE_ATOM_VDW_RADII = {

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.

2 participants