MDAnalysis / membrane-curvature

MDAnalysis tool to calculate membrane curvature.
https://membrane-curvature.readthedocs.io/
GNU General Public License v3.0
29 stars 6 forks source link

Requested changes to initial refactor #38

Closed ojeda-e closed 3 years ago

ojeda-e commented 3 years ago

This PR fixes #33.

Changes included:

^   o ___ o ___ o ___ o ___ o ___ o ___ |
|   | (6) |     | (7) |     | (8) |     |
|   o ___ o ___ o ___ o ___ o ___ o ___ |
|   |     |     |     |     |     |     |
|   o ___ o ___ o ___ o ___ o ___ o ___ |
y   | (3) |     | (4) |     | (5) |     | 
|   o ___ o ___ o ___ o ___ o ___ o ___ |
|   |     |     |     |     |     |     |
|   o ___ o ___ o ___ o ___ o ___ o ___ |
|   | (0) |     | (1) |     | (2) |
v   o ___ o ___ o ___ o ___ o ___ o ___ |
    0.   0.5    1.   1.5.   2.   2.5.   3.
    <----------------- x --------------->

For grid with 9 lipid types: Bead 0 mapped to [ 0 0 ], with Coordinates ( 0 , 0 ) Bead 1 mapped to [ 2 0 ], with Coordinates ( 1 , 0 ) Bead 2 mapped to [ 4 0 ], with Coordinates ( 2 , 0 ) Bead 3 mapped to [ 0 2 ], with Coordinates ( 0 , 1 ) Bead 4 mapped to [ 2 2 ], with Coordinates ( 1 , 1 ) Bead 5 mapped to [ 2 2 ], with Coordinates ( 1 , 1 ) Bead 6 mapped to [ 0 4 ], with Coordinates ( 0 , 2 ) Bead 7 mapped to [ 2 4 ], with Coordinates ( 1 , 2 ) Bead 8 mapped to [ 4 4 ], with Coordinates ( 2 , 2 )

and the equivalent to the grid of 25 lipids.

This PR may also fix:

27 since function def_all_beads was deleted after replacing MDtraj by MDAnalysis.

28 since function core_fast was deleted after refactoring.

32 since function def_all_beads was deleted after refactoring and replaced by direct selection using MDAnalysis.

and #16

Questions: (possible minor change)

codecov[bot] commented 3 years ago

Codecov Report

Merging #38 (9b53141) into main (8b87789) will not change coverage. The diff coverage is n/a.

ojeda-e commented 3 years ago

Hi @lilyminium, could I have a review in this PR, please :) Thanks.

pep8speaks commented 3 years ago

Hello @ojeda-e! 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! :beers:

Comment last updated at 2021-06-30 01:19:26 UTC
ojeda-e commented 3 years ago

Thanks for the comments @fiona-naughton. Changes added.

ojeda-e commented 3 years ago

Thanks for making this change! Could you please just remove repeated values like (0, 0) -- although to be honest the second 5x5 grid isn't necessary.

I added the 5x5 as suggested in previous PRs. If you are ok with it with different values excluding (0,0). @lilyminium Maybe inverted values? As in

(4, 0), (3, 0), (2, 0), (1, 0), (1, 1),
(4, 1), (3, 1), (2, 1), (1, 1), (0, 2),
(4, 2), (3, 2), (2, 2), (1, 2), (0, 3),
(4, 3), (3, 3), (2, 3), (1, 3), (0, 5),
(4, 4), (3, 4), (2, 4), (1, 4), (0, 4)

I would prefer to keep it even if not necessary.

lilyminium commented 3 years ago

Maybe inverted values?

I think those are just the same values in a different order, but sure, we can leave the grid in as-is so it's easier to modify for future tests :)