mittinatten / freesasa

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

Ported over the vdw radii for all elements from gemmi. #71

Closed danny305 closed 2 years ago

danny305 commented 2 years ago

Manually entered each radii to address issue #70

These should definitely be double checked by a second set of eyes.

mittinatten commented 2 years ago

That was quick! I'll look through this later today or during the weekend. It seems the only tests that fail are those for pdb code 1d3z, which is used an example of a structure with hydrogens, and one test for verifying that hydrogen gets the right radius (do you have access to the Travis logs above?). The 1d3z tests only check that you get the correct total, to catch regressions. This is not a regression but a change due to a new hydrogen radius, so the reference value should be updated. Possibly there will be a failing test in the Python module down the line too. So you probably just have to change the reference value two places in the code.

These are the tests that fail (I think these are all)

mittinatten commented 2 years ago

I wonder if it's correct to have 1.2 Å for hydrogen, Mantina et al (https://www.ncbi.nlm.nih.gov/pmc/articles/PMC3658832/), which is the newest paper cited by Wikipedia, ends up with 1.10 Å (that's were I got the original value from). If we can keep that, and 1.83 for Br and 2.17 for Sn, that would make this a non-breaking change. (It seems I actually used the old Bondi radii, and not the "final" ones from the table in the paper https://www.ncbi.nlm.nih.gov/pmc/articles/PMC3658832/table/T9/?report=objectonly. Can't remember any reason why.)

danny305 commented 2 years ago

Yeah. I have no idea how gemmi calculated the radii for all of those missing elements but im glad someone took a stab at it.

I trust the people at gemmi and they provide good default to use until experimental data becomes available and wikipedia is updated.

I have fixed the radii you suggested (H, Sn, Br) and removed the duplicate for Hg.

Let me know if there is anything else.

coveralls commented 2 years ago

Coverage Status

Coverage increased (+0.01%) to 97.73% when pulling ab2de9d6fa8e76d7be250f4f415b7e2cb4e076d1 on danny305:feature/vdw_radii into 744493386524da6d62fc5cbd2d876524c30184cd on mittinatten:master.

mittinatten commented 2 years ago

I missed this in my previous check, but Li should be 1.81: https://www.ncbi.nlm.nih.gov/pmc/articles/PMC3658832/table/T12/

There was one other change that I missed in the first round, Mg went from 1.74 to 1.73, but I think 1.73 is correct. Must have been a typo. We'll just add that to the changelog, it's so minor calling it "breaking" would be a bit much I think 😄 .

Otherwise I think we're set!