search

mittinatten / freesasa

C-library for calculating Solvent Accessible Surface Areas
http://freesasa.github.io/
MIT License
103 stars 37 forks source link

Flesh out Van Der Waals radii for all elements #70

Closed danny305 closed 2 years ago

danny305 commented 2 years ago

So your current element radii are incomplete.

I have been searching for a source of all element vdw radii and I could not find something online.

Then, surprisingly, I realize that gemmi solved this problem: https://github.com/project-gemmi/gemmi/blob/e784eac007b8f74485c02e3c135c81fef48e1ef8/include/gemmi/elem.hpp#L156

I am going to transition over the radii to these values on my freesasa fork. The good ole fashion copy-paste.

Would you like me to submit a PR for this? there are a few elements in your current list where the radii values are slightly different, which is why I am asking you how you would like to proceed.

mittinatten commented 2 years ago

Hi! Good suggestion! Copying and pasting from Gemmi/Wikipedia is probably not a terrible idea. It is a breaking change, but quite minor. I think it's better to have one consistent set, than keep the previous values. (These radii are pretty approximate anyway, what is the "correct" value depends quite a lot on chemical environment. This is why I went for the ProtOr set for the regular amino acid atoms, which takes that into account and allows you to ignore hydrogens at the same time).

A PR would be great, probably some of the unit tests will need to be rewritten as well, I can help you with that.

mittinatten commented 2 years ago

I merged the PR and added a mention to the CHANGELOG (and credited you for the new features). Thanks for your contribution!

danny305 commented 2 years ago

Thanks Simon!

Im sure I will submit more issues and pull request and keep our collaboration going.