Requested changes to initial refactor

[ojeda-e]

This PR fixes #33.

Changes included:

• pytest.mark.xfail test of negative coordinates in grid_map.
• 9- and 25-grid combined into one function with pytest.parametrize.
• Test_mapper changed by adding assertion of dummy coordinates.
• Test with grid_map using 6 unit cells in grid of 9 lipids as shown below:

^   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)

• In line 172 and 177, I could simplify to factor = 1
  and then in lines 174 and 179, respectively.
  assert grid_map(dummy_coord, 1) == dummy_coord
assert grid_map(dummy_coord, 2) == (dummy_coord[0]*2, dummy_coord[1]*2)

[codecov]

Codecov Report

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

[ojeda-e]

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

[pep8speaks]

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! 🍻

Comment last updated at 2021-06-30 01:19:26 UTC

[ojeda-e]

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

[lilyminium]

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.

[ojeda-e]

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]

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 :)