AnalysisBase

[ojeda-e]

This PR is an initial suggestion to fix #41

• Methods included in the initial analysis base:

• [x] __init__

• [x] _prepare

• [x] _single_frame

• [x] _conclude

• Some additional comments

This first AnalysisBase was build under the following assumptions:

1. The atoms of reference in AtomGroup remain the same during the n_frames of the trajectory.
2. lipid translocation does not occur during the trajectory. (Although I am not considering bilayers, this reinforces 1)
3. By introducing a grid of bigger size than the simulation box, the calculation of H and K may be jeopardized by the introduction of np.nans. This selection will certainly generate np.nans in the calculation of the gradient, which ultimately will affect the averaged grids. Hence, the attempt of warning the user (in __init__).
4. The overall result is the averaged np.array of surface, K, and H over frames. (in _conclude)
5. The calculation of surface, H, and K, is performed in every frame (in _single_frame).
6. The respective values of surface, H and K and their normalized grids are initialized (in _prepare). They are a tuple of np.arrays where the first element contains the respective values, and the second element counts the elements when defined.
7. To sum and normalize values, the _output function was introduced. (But I am not sure this is something people would agree with)

As discussed in #21 with @lilyminium, this analysis base will potentially replace core.py.

[codecov[bot]]

Codecov Report

Merging #43 (9451b0c) into main (3e87eec) will increase coverage by 32.40%. The diff coverage is 100.00%.

[ojeda-e]

Hi @lilyminium I updated the AnalysisBase with the suggested changes and other minor improvements and corrections. I also changed the position of the argument selection in the derive_surface function just for consistency.

[lilyminium]

This design is looking really, really good @ojeda-e. I don't have much to say except that some lines are looking quite long so you may want to check that you're following PEP8. One trick could be to make get_z_surface a method in the class, since it uses attributes of self so often -- but that's minor.

I guess the proof is in the pudding -- would you like to start writing tests to see how it works? :-)

[lilyminium]

Also great idea to switch the order of the arguments to derive_surface, the new order feels much more intuitive!

[ojeda-e]

I guess the proof is in the pudding -- would you like to start writing tests to see how it works? :-)

Yes! :D I will open an issue for the respective tests and will work on them after solving pbc conditions in surface.py.

If you are ok with it, I will skip making get_z_surface as a method. At least temporarily. :) I will push the final version briefly.

[ojeda-e]

@lilyminium re-reading your comment makes me wonder if you would prefer the tests in this PR or in a separate issue. Apologies I wrongly assumed it was a separate PR. What do you think works best?

[lilyminium]

I would personally prefer in this PR, just so we make sure that everything in the main branch always works :-)

On Jul 7, 2021, at 6:30 PM, Estefania Barreto-Ojeda @.***> wrote:

 @lilyminium re-reading your comment makes me wonder if you would prefer the tests in this PR or in a separate issue. Apologies I wrongly assumed it was a separate PR. What do you think works best?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[lilyminium]

Another catch is that I think it’s actually called MDAnalysis==2.0.0b0

@IAlibay could you confirm?

On Jul 9, 2021, at 12:53 PM, Rocco Meli @.***> wrote:

 @RMeli commented on this pull request.

In devtools/conda-envs/test_env.yaml:

• sphinx

  • • MDAnalysis==2.0.0-dev0 I think you need to install it explicitly via pip:

⬇️ Suggested change

• • MDAnalysis==2.0.0-dev0
• • pip:
• • MDAnalysis==2.0.0-dev0 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[RMeli]

@lilyminium I had this feeling and I was looking for the announcement mentioning the beta package name... Do you know where I can find it (for future reference)? It is not with the usual blogs. Was it just on Twitter?

[IAlibay]

@lilyminium I had this feeling and I was looking for the announcement mentioning the beta package name... Do you know where I can find it (for future reference)? It is not with the usual blogs. Was it just on Twitter?

We never got around to it, we were going to announce it with the MDAnalysis workshop materials but unfortunately that got stalled.

edit: also yes, can confirm it's MDAnalysis==2.0.0b0

Long term we should have a CI runner that directly installs the dev version though.

[lilyminium]

Sorry I didn't mean to remove those review requests... hazards of githubbing on phone 🤦‍♀️

[ojeda-e]

Thanks, @lilyminium, @IAlibay, and @RMeli for your comments.

I just wanted to add an initial version of the tests with previously used dummy_arrays and a Universe with .gro and .xtc files. They passed, thanks again!

[pep8speaks]

Hello @ojeda-e! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file membrane_curvature/base.py:

Line 21:15: W291 trailing whitespace Line 35:47: W291 trailing whitespace Line 60:40: W291 trailing whitespace Line 61:54: W291 trailing whitespace Line 63:45: W291 trailing whitespace Line 66:48: W291 trailing whitespace Line 99:86: W291 trailing whitespace

• In the file membrane_curvature/tests/test_membrane_curvature.py:

Line 301:5: E303 too many blank lines (2) Line 309:53: W292 no newline at end of file

Comment last updated at 2021-07-14 01:04:48 UTC

[ojeda-e]

As suggested by @IAlibay, docstrings were added and suggested changes that improve the readability of the code. :)

[lilyminium]

Hi Estefania,

I believe the DensityAnalysis in MDAnalysis deals with a similar problem. Perhaps have a look at how the box is defined there? If I recall correctly, the box doesn’t change but you may get warnings. @.*** would know more as the one who wrote it!)

https://docs.mdanalysis.org/1.1.1/documentation_pages/analysis/density.html

https://docs.mdanalysis.org/1.1.1/_modules/MDAnalysis/analysis/density.html#DensityAnalysis

Cheers, Lily

On Jul 11, 2021, at 10:00 AM, Estefania Barreto-Ojeda @.***> wrote:

 @ojeda-e commented on this pull request.

In membrane_curvature/base.py:

• y_range=None,
• pbc=True, **kwargs):
• super().init(universe.universe.trajectory, **kwargs)
• self.selection = universe.select_atoms(select)
• self.pbc = pbc
• self.n_x_bins = n_x_bins
• self.n_y_bins = n_y_bins
• self.x_range = x_range if x_range else (0, universe.dimensions[0])
• self.y_range = y_range if y_range else (0, universe.dimensions[1])

• Raise if selection doesn't exist

• if (self.selection.n_residues) == 0:
• raise ValueError("Invalid selection. Empty AtomGroup.")

• Raise if range doesn't cover entire dimensions of simulation box

  I think that's something I didn't pay too much attention to (and I didn't clarify, my bad). One of the assumptions I had is that calculation of membrane curvature is performed in equilibrated systems. If the box shrinks or expands too much (one unit cell in the grid), the calculation wouldn't be accurate.

The reason why I introduced the restrictions (which I changed now to a warning message) is that (1) if the surface is derived from a portion of the membrane, the calculation of curvature in the edges will be inaccurate because the calculation of a gradient runs over adjacent values. On the other hand, (2) in the scenario in which the grid is too big, unnecessary np.nans will be introduced. It means that, again, the undefined adjacent values will have a negative impact on the calculation of curvature.

I don't see a straightforward solution for NPT systems unless the calculation returns a different grid per frame and then results must be normalized over grids of the same size, and I don't have a clear idea, at least right now, of how to keep track of such variation over frames. Do you have any suggestions here?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[ojeda-e]

This PR includes the AnalysisBase with respective docstrings and tests. To limit the extent of this PR, the limitations pointed out by @IAlibay of possible NPT boxes will be in issue #47. PBC conditions will be addressed in issue #36

I would appreciate a final review here @IAlibay @orbeckst @fiona-naughton

[orbeckst]

That was an important merged PR, well done!