Closed ojeda-e closed 2 years ago
Check out this pull request on
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
@lilyminium @orbeckst would you please help me with a quick review here? Thanks :)
View / edit / reply to this conversation on ReviewNB
hmacdope commented on 2021-09-29T04:29:19Z ----------------------------------------------------------------
Perhaps you can do this with a numpy or scipy function and get it to spit out 1% at the end. Something like np.std(axis=0)
View / edit / reply to this conversation on ReviewNB
hmacdope commented on 2021-09-29T04:29:20Z ----------------------------------------------------------------
You can also do an equality check on the residues, should be illustrative for a user to see ==
View / edit / reply to this conversation on ReviewNB
hmacdope commented on 2021-09-29T04:29:21Z ----------------------------------------------------------------
As noted above I would directly test for equality here. I would also remove the word "new" from variable names and be more specific eg leafletfinder_leaflets
ojeda-e commented on 2021-09-29T21:58:01Z ----------------------------------------------------------------
Thanks, I changed the name of the variables but left the equality test only for the previous comment. My intention here is more to read the elements of each leaflet. Hope this works too. :)
View / edit / reply to this conversation on ReviewNB
hmacdope commented on 2021-09-29T04:29:21Z ----------------------------------------------------------------
As with other notebook a quick intro to what the nbins parameter is would be helpful.
ojeda-e commented on 2021-09-29T21:58:18Z ----------------------------------------------------------------
Yes, thanks for this, it's a very good point.
View / edit / reply to this conversation on ReviewNB
hmacdope commented on 2021-09-29T04:29:22Z ----------------------------------------------------------------
perhaps "curv_upper_leaflet_f47" -> "curv_upper_leaflet_f_4_and_7" as I thought it was 47.
ojeda-e commented on 2021-09-29T22:01:18Z ----------------------------------------------------------------
Thanks, will update.
View / edit / reply to this conversation on ReviewNB
hmacdope commented on 2021-09-29T04:29:22Z ----------------------------------------------------------------
What is each matrix element representing.
View / edit / reply to this conversation on ReviewNB
hmacdope commented on 2021-09-29T04:29:23Z ----------------------------------------------------------------
Add detail about units for each curvature here.
View / edit / reply to this conversation on ReviewNB
hmacdope commented on 2021-09-29T04:29:24Z ----------------------------------------------------------------
Line #5. MembraneCurvature is a tool to calculate membrane curvature.
Same here for general docstirng.
added
View / edit / reply to this conversation on ReviewNB
hmacdope commented on 2021-09-29T04:29:25Z ----------------------------------------------------------------
As with other PR is there a paper linking gauss curvature and elasticity?
Thanks @hmacdope! I added the suggested changes. A link to further reading of membrane curvature is still missing. There is not much material out there, but maybe linking this may work? Not sure that's what you had in mind, though.
Will wait for @orbeckst or @lilyminium to confirm I can merge.
I like that review. Perhaps linking to it as an introduction to curvature is a good idea. :)
On Wed, Sep 29, 2021 at 3:18 PM Estefania Barreto-Ojeda < @.***> wrote:
Thanks @hmacdope https://github.com/hmacdope! I added the suggested changes. A link to further reading of membrane curvature is still missing. There is not much material out there, but maybe linking this https://www.researchgate.net/figure/Measures-of-surface-curvature-a-The-principal-curvatures-are-calculated-from-the_fig6_321165318 may work? Not sure that's what you had in mind, though.
Will wait for @orbeckst https://github.com/orbeckst or @lilyminium https://github.com/lilyminium to confirm I can merge.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/MDAnalysis/membrane-curvature/pull/78#issuecomment-929844091, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF3RHCYLS3T6CH3ZV4Z3XATUEKOR5ANCNFSM5EPZXWTQ .
-- Hugo MacDermott-Opeskin PhD Candidate, RSC ANU Email: @. @.>
Thanks, I changed the name of the variables but left the equality test only for the previous comment. My intention here is more to read the elements of each leaflet. Hope this works too. :)
View entire conversation on ReviewNB
After fixing #70 , the tutorial of the membrane-only system needed some updates. This version should close #74.
Changes in this PR:
I'm tagging @lilyminium and @orbeckst who usually review my PRs, but more reviewers are welcome. :) Thanks.