-
Notifications
You must be signed in to change notification settings - Fork 652
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
Accessible Surface Area calculations #4417
base: develop
Are you sure you want to change the base?
Conversation
Hello @JureCerar! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2024-07-09 22:52:39 UTC |
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.
Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on GitHub Discussions so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS
as part of this PR.
Linter Bot Results:Hi @JureCerar! Thanks for making this PR. We linted your code and found the following: Some issues were found with the formatting of your code.
Please have a look at the Please note: The |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4417 +/- ##
===========================================
+ Coverage 93.61% 93.63% +0.02%
===========================================
Files 171 172 +1
Lines 21243 21394 +151
Branches 3934 3970 +36
===========================================
+ Hits 19886 20032 +146
Misses 898 898
- Partials 459 464 +5 ☔ View full report in Codecov by Sentry. |
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.
I'm going to put a blocker here because I'm not really sure how to best go about this one - given that this is acknowledged as a modification of #4025 and that @pegerto has their own MDAKit for this now - https://github.com/pegerto/mdakit_sasa/ (which I assume is also based off their own work in #4025).
Thoughts everyone? Particularly pinging @MDAnalysis/coredevs
It is unfortunate that the MDAKit was not more widely publicized, leading to duplication of effort. @JureCerar, would it make sense for you to contribute the calculation of relative accessible surface area to the MDAKit (assuming it has not yet been implemented)? |
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.
@JureCerar this is an impressively complete contribution with documentation and extensive testing.
You mentioned that you based your work on @pegerto's PR #4025. Could you please comment on which parts of PR #4025 you used? This is important because some parts of PR #4025 may have been inspired by other work and we need to understand where code comes from.
Have you compared output and performance of the code here to mdakit-sasa?
If we proceed with the PR then we would also add @pegerto as an author to give proper credit. We would also add a note to the docs that mdakit-sasa exists as an alternative.
@orbeckst I used and modified the main surface calculation code. As far as I know, the code from #4025 is copied from BioPython/SASA which is under BSD 3 license. Only |
Thank you for the details. BSD 3 would be ok. Have you compared output and performance of the code here to mdakit-sasa? What does your code and mdakit-sasa have in common, where do they differ? |
I checked the code. The main difference is mdakit-sasa is a wrapper for FreeSASA package. So the underlying algorithm is different. FreeSASA uses Lee-Richards algorithm where as this code uses Shrake-Rupley algorithm. Performance wise I did not test it. But I figure FreeSASA (mdkit-sasa) is faster, as it's implemented in C? It's hard to make head-to-head comparison as the algorithm is different. This implementation finishes a 10 frame trajectory of a ~400 residue protein in about a minute or two, which I think is a reasonable speed. In any case, precision can be lowered if speed is needed. Output wise, the result (i.e. area) is the same regardless of the method or package used. Here it's also implemented the Relative Surface Area calculation which is a very useful to have when calculating protein surface properties. I guess it could also be implemented in mdkit-sasa? Just as a side note. I similarly tried writing a wrapper for |
Hi All. As mention by @JureCerar mda_kit wraps the implementation FreeSASA in the BaseAnalysis class, and this kit is very simple as all the heavy lifting is done by FreeSASA:
Regarding performance, perforce is heavily driven by parametrisation, in the case of the Shanke-Rupley the number of points of the spheres are a main parameter if you use Gromacs SASA calculation the default parameters use very few points, FreeSASA have a nThread implementation builtin, but the kit do not implement parallelisation over multiple frames at the moment. The reason for switching the PR to a kit initially was to separate FreeSASA dependency from core. Let me knot if there is something I can help with. Regards. |
Thanks @pegerto and @JureCerar ! Some of the developers are currently discussing how to best move forward. We'll keep you updated. Thank you for your patience! |
This might be very easy once we merge PR #4162 . |
Please see #2439 (comment) |
Fixes #2439
Changes made in this Pull Request:
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4417.org.readthedocs.build/en/4417/