-
Notifications
You must be signed in to change notification settings - Fork 27
Conversation
Pulling master into issue_68 for testing purposes
…lts. Should test further.
…eeprank.features.rst
Pull Request Test Coverage Report for Build 1127779947
💛 - Coveralls |
|
||
class Edesolv(FeatureClass): | ||
|
||
# init the class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please leave a comment about the equations and references used for calculating Edesolv.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean in the class docstring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improved in 6fed0fc . Please let me know if it's not satisfactory yet.
deeprank/features/Edesolv.py
Outdated
chainA = chains[0] | ||
chainB = chains[1] | ||
|
||
# Make free_structure fake object and translate the chains away from each other |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split the chains to independent pdbs and use those for SA calculation of free chains
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that just cause extra unnecessary I/O (or memory) usage? Can I ask you why this is not fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extra memory usage is quite cheap.
The main issue here is your method cannot make sure the chains are really away from each other, and also it will cause troubles when applied to multiple-chain complexes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Will change it.
self.edesolv_data = {} | ||
self.edesolv_data_xyz = {} | ||
|
||
for key, coords in zip(keys, xyz): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loop here is using the contact atoms to calculate Edsolv. The contact atoms are dependent on distance cutoff. So the Edsolv will also be dependent on distance cutoff. It is not a great idea to calculate Edsolv in this way.
You could keep these code and rewrite one as the way used in Haddock, then compare the results to check the difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So would you want me to calculate the Edesolv for each atom in the proteins (as HADDOCK does), although in the end we only use the ones in the grid? Do you want it to be done constitutively or just for the comparison with HADDOCK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, I forgot we need only the atoms in the grid. It's fine then to calculate the Edesolv for only contact atoms with one place to improve:
Make sure the distance_cutoff for defining contact is same as the one used to get grid atoms. You should transfer the value in DataGenerator.create_database(contact_distance=8.5)
to the "# get the contact atoms" step in this code.
To make sure the results are correct, you also need to compare the results with Haddock by giving a big enough distance cutoff to get all atoms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
Thank you very much for the tips, will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, thanks.
I summarise the major comments below:
- Try to calculate the Edsolv as how Haddock does and get rid of the dependence on contact atoms
- Try to use absolute difference rather than pearson correlation coefficient to compare the results between the code and Haddock
- Write an unit test to test the code
…d chain2 arguments to compute_feature
For point 3, isn't it better if I simply add Edesolv as a feature in test_generate.py? In this way, it will be tested along with the other features and there will be no need to change its specific unit test. |
Yep, makes sense :-) |
@DarioMarzella Please leave me a message when all requested changes have been handled :-) |
Yes, I am waiting for the comparison with HADDOCK to update you :) |
I just got the result, and seems like the Edesolv I get from DeepRank by setting a grid big enough to take the whole protein correlates a bit worse with haddock than the one I previously got by taking into account the interface only. This makes me wonder if HADDOCK is using the whole proteins or the surface atoms only for the Edesolv. Last result (using all-atom Edesolv): Previous results (using interface-only Edesolv): |
@DarioMarzella Thanks for the results! If we're using the same equations and parameters as Haddock, the results should be same or the difference should be at the level of precision, e.g. 0.01(?). Did you check the Haddock code? If not, you'd better to have a look and also get clear with your wonder "This makes me wonder if HADDOCK is using the whole proteins or the surface atoms only for the Edesolv." (I don't remember and cannot provide a direct answer here :-( And note the difference might be also from the calculation of surface area, Haddock does not use biopython for that. If the major part of difference is from area calculation, it's fine then and we should leave a note in the documentation. |
This repo is going to be archived #260. So closing this PR now. |
Energy of desolvation currently working in this branch (complete issue: #issue68).
The organization of
deeprank/features/Edesolv.py
though might not be great, so I would appreciate comments on that (if needed).