search

mittinatten / freesasa

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

Removing false positive detection of H-atoms #33

Closed arghya90 closed 6 years ago

arghya90 commented 6 years ago

In the function 'freesasa_pdb_ishydrogen', the method of detecting h-atoms resulted into some non-H atoms being discarded. For example, oxygen atoms (OD, OD1, etc.) and carbon atoms (CD, CD1, etc.). I have added an extra line of if-statement that corrects for that.

Kindly consider.

mittinatten commented 6 years ago

Hi and thank you! I'm not sure this does what you intended it to do, it causes a bunch of errors in the automated tests, and the code looks like it will miss a lot of true positives.

Do you have a sample file, or even line from a pdb file, that fails?

arghya90 commented 6 years ago

Dear Author

Thanks for the response.

So I tested freeSASA with my modifications on a PDB file where by having it read the radii from occupancy column. I have attached a sample file here. It has an extension of PRQ (not PQR) and a LOG output from freeSASA. When I run it with such a file (I have around 75 of them for different proteins), the runs do not show any error. And that is all I used for testing the modification.

I am not sure how adding a single if-statement line in the function (line 270 in pdb.c) cause error. Probably I am missing something there.

I would be happy to be of help if needed.

Thanks again, Argo

On Nov 8, 2017, at 12:07 PM, Simon Mitternacht notifications@github.com wrote:

Hi and thank you! I'm not sure this does what you intended it to do, it causes a bunch of errors in the automated tests, and the code looks like it will miss a lot of true positives.

Do you have a sample file, or even line from a pdb file, that fails?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mittinatten/freesasa/pull/33#issuecomment-342885640, or mute the thread https://github.com/notifications/unsubscribe-auth/Ab271YPxUM2sQCiw4wRCy21Ma77WV_Jmks5s0d_agaJpZM4QWq-u.

mittinatten commented 6 years ago

Hi, seems the attachments didn't make it through to Github. If you could just insert a few of the offending lines here in the thread I will have something to work with (paste it as code to get the whitespace right).

This very basic test fails: https://github.com/mittinatten/freesasa/blob/master/tests/test_pdb.c#L87 in your code. The other errors seem to be that for the PDB files in the test suite with hydrogens, hydrogens are not skipped when they're supposed to and you get the wrong number of atoms. If you try 1D3Z with your code for example (can be found in tests/data/), I think you will get a bunch of warnings.

mittinatten commented 6 years ago

Closing this for now. From what you describe it sounds like you might have been using non-conforming PDB files rather than there being a bug. Feel free to reopen if there's any further information or development.