Initial refactor and test added [Fixes #33]

[ojeda-e]

Helps to fix #33. In this first version of the refactored code, the calculation of curvature doesn't evaluate per leaflet, and instead, it runs for a surface defined by selected atoms. (More general)

Changes in this PR:

• [x] Surface derived from direct selection using MDAnalysis.
• [x] Deleted functions def_all_beads, curvature and core_fast.
• [x] Function grid_map to map coordinates to grid added.
• [x] In grid_map arguments as np.array / tuples instead of topologies.
• [x] Replaced MDtraj by MDanalysis 100%.

As suggested here, tests with toy model using pytest.mark.parametrize added:

• [x] test_grid_map_small_9grid, using toy model in small grid of 9 lipids in a 3x3 grid, with x values of 0, 1, 2, and y values of 0, 1, 2.
• [x] test_grid_map_25grid, using toy model in small grid of 25 lipids in a 5x5 grid, with x values of 0, 1, 2,3,4 and y values of 0, 1, 2,3,4.

This PR may also fix

• 27 since function def_all_beads was deleted after replacing MDtraj byMDAnalysis`.

• 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`.

Edit: This PR also fixes #16 .

[codecov[bot]]

Codecov Report

Merging #34 (8569cf0) into main (bf6019b) will increase coverage by 9.39%. The diff coverage is 73.33%.

[ojeda-e]

Thanks for your comments @orbeckst There are two main reasons for all the changes here introduced.

• The first one and most relevant, is that in the previous version of my code, topologies and trajectories fromMDtraj were parsed in three of the functions: def_all_beads, core_fast_leaflets, and core_fast. With the change from MDtraj to MDAnalysis, the arguments of def_all_beads disappeared, core_fast core_fast_leaflets significantly changed.
• The second reason is that in previous PRs (see PR #31 and #30) the tests I was asked to provide were with dummy coordinates, to be more readable. Which is completely ok, but makes more sense only after refactoring. As I mentioned above, In the previous version of the code, the arguments parsed in 3 of the functions were MDtraj topologies and trajectories that I can't build with numpy arrays. Only after this refactoring, and having MDAnalysis instead, that type of test makes more sense.
• Additionally, since the previous PRs I was asked to have tests using dummy coordinates and the ones I provided with grofiles were getting too complicated to read, I simplify the tests in this new version of the code.

In the spirit of moving forward, I added changes in this PR that wrap up the previous limitations and allows me to progress. If further tests are needed, now with this refactored version providing dummy coordinates will be possible. I feel I haven't made progress because of the tests that have been asked previously, which were going to change no matter what with this PR. If it's better, I can put the big systems tests for two of the functions, mean_curvature and gaussian_curvature, which are essentially the only two functions that remain the same after refactoring.

[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! :beers:

Comment last updated at 2021-06-25 03:37:02 UTC

[ojeda-e]

Hi @lilyminium Lily, thanks for your review. Regarding this comment (and below)

Currently core_fast_leaflet does three things:

• gets positions for each atom in the atom group for each frame
• identifies the grid cell for each coordinate (currently grid_map)
• calculates the average z for the atom group

Could you please break this down into three functions, and write a new function that simply calls them (just for the regression tests?) I would be keen to have these as divorced from either MDTraj or MDAnalysis as possible and work only with numpy arrays, which are generally more versatile. The overall function could grab the positions from the AtomGroup and feed it into the functions instead.

If it works for you, I'll add these remarks as new issues instead of adding them in this PR. The changes are getting a bit too long. Would that work? If yes, then it'll be happy to submit a PR with the requested changes.