🧬 helanal refactor 🧬#2622
Conversation
|
@wg150 @fiona-naughton I would love your thoughts on this as it is now and anything you would add! |
Codecov Report
@@ Coverage Diff @@
## develop #2622 +/- ##
===========================================
- Coverage 92.67% 92.23% -0.44%
===========================================
Files 184 185 +1
Lines 24057 24300 +243
Branches 3102 3148 +46
===========================================
+ Hits 22294 22413 +119
- Misses 1717 1822 +105
- Partials 46 65 +19
Continue to review full report at Codecov.
|
|
This looks like good code, but someone who actually knows the science ought to review it |
|
Hm, ok, something to fix here. (x-axis is time)
|
|
In general your vector and angle calculations aren’t respecting PBC. Try
unwrapping and see if this fixes it?
…On Mon, Mar 16, 2020 at 20:11, Lily Wang ***@***.***> wrote:
Hm, ok, something to fix here. (x-axis is time)
[image: Screenshot 2020-03-17 at 7 09 43 AM]
<https://user-images.githubusercontent.com/31115101/76795883-5f930680-681e-11ea-8b7f-6ac703ec0b95.png>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2622 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACGSGB5TCWZCGFIEJAGCNU3RH2BXFANCNFSM4LHDTIOA>
.
|
|
These are already preprocessed and unwrapped -- I think the 30 degrees is swinging around to 150 because I'm only calculating the angle from one direction. But yes I should put in PBC |
|
I'm going to have to come up with better tests than just comparing to old helanal's output... Is this what Edit: x-axis is now residue.
Should |
fiona-naughton
left a comment
fiona-naughton
left a comment
There was a problem hiding this comment.
Looks good! I think you've pretty much covered all the major changes I wanted to see :) I've added a couple comments; several of them are particularly related to the algorithm/geometry involved which I'm not 100% sure I followed completely, so do ignore me if I'm wrong.
A couple more comments on the notebook I think might make things clearer:
- On the first figure, it's perhaps not immediately clear what plane each the left and right are drawn in. I assume this is what the
local_axesare trying to show, but it took me a bit to realise that it's coming out of the page on the left rather than some kind of radial vector. Maybe have a picture of a helix on the side with the planes slicing through it, or something? - The second/third figures might also benefit from showing a helix, just to make it clearer at a glance which arrows correspond to what
- Could show the screw angles/rotation vectors on the third figure
|
Re: tests/testing data - those plots do look a little suspicious, though I'm not immediately sure what exactly would be the issue. What input were you using? Doesn't look like the right number of residues for the one the tests use. Also, might be worth adding a test for when a different |
|
Thanks for the review @fiona-naughton! You brought up many of the things I wondered about myself. The inputs are helices from my own trajectories (this refactor was borne out of necessity) but I can put some graphs together from the test data.
Good idea, I think in general the module needs more tests than the old one had. |
lilyminium
force-pushed
the
helanal
branch
2 times, most recently
from
af183fb to
b9926aa
Compare
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
TODO: Deprecate the old helanal module.
… to global_axis and ref_axis
|
Hello @lilyminium! 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 2020-07-19 00:53:39 UTC |
|
Sorry for another delay - looks good to me! (though I see one of the travis jobs has failed, but I don't know enough to know what's gone wrong there...) |
|
Thank you @fiona-naughton!!! Thank you for getting through this 😁 😁 😁 Travis was failing on some double RDF docs, I've fixed that now. |
|
@fiona-naughton can you please squash-merge this and rewrite the commit message using the information from the PR? Please include the "Fixes" line, and clean up the commit message lines that come from the individual commits. (E.g., leave out any "fixed pep8", "now finally got X working" and all other "PR internal stuff".) The guiding principle is simply that this single commit message should have all the relevant information for the whole thing. Thanks! Well done, I am glad that this PR finally sees the light of day! EDIT: added a few please and thank yous... sorry, it's late |
|
@lilyminium can you please
Thank you! |
* deprecate helanal, to be removed in 2.0.0 (use analysis.helix_analysis in 2.0 from PR #2622) * add deprecation warnings to analysis.helanal * update CHANGELOG
* deprecate helanal, to be removed in 2.0.0 (use analysis.helix_analysis in 2.0 from PR #2622) * add deprecation warnings to analysis.helanal * update CHANGELOG
Fixes MDAnalysis#2452 * added new helanal using AnalysisBase * deprecated old helanal * added universe_from_origin * updated to use np clip * added new local_screw algorithm * changed screw angles to projection onto cross-axis
6 participants
Fixes #2452
Changes made in this Pull Request:
Well, it produces the same numbers as the oldChanged the algorithm for calculating screw angles. A few things left (which the old module didn't implement):helanal.Demo notebook here. It also has pictures of the attributes, I would appreciate feedback on if they make it clear what HELANAL actually calculates.
PR Checklist