Some of the missing features that would be nice to solve before the next release are:

• The membrane-curvature PyPi page has no project description.
• There is no CHANGELOG. After fixing #70 it would be nice to keep track of the main changes.
• Tutorials are still pending to reach 100%.

To-do for the next release (0.0.2):

• Add description in setup.py.
• Add CHANGELOG.
• Fix tutorial membrane only. (#74)
• Finish second tutorial. (Currently PR #68)
• Add CITATION.cff.

As pointed out by @orbeckst, after initial refactor in #34 the grid is assumed to be square. The grid should accept at least rectangular shapes.

To do:

• Define the size of the grid by at least x and y.
  Suggestion: box_size = max(u.dimensions[0], u.dimensions[1])
• Add xfail for orhtorombic/triclinic box.

Currently, there is no information about the performance of membrane curvature. It would be nice to have a page in the documentation that addresses this matter. Although the idea is not very well defined yet, some guidelines for an initial version of the benchmark are:

• Performance over number of lipids in single-frame systems.
• Performance over number of frames with a fixed number of lipids.
• Keep the list of assumptions regardless of the approach.
• Consider dummy Universes.
• MDAnalysis uses ASV (Check code here)

Thanks @hmacdope, @lilyminium and @richardjgowers for the suggestions.

A first step is probably to upload @ojeda-e's existing, working code, into the directory structure laid out by the cookiecutter.

Import membrane_curvature (Line 16) seems to be the problem.
Docs build otherwise.

Any suggestion @lilyminium on how to solve this issue?

As noted by @IAlibay in comments on PR #59 , there's an inconsistency in the licensing:

• docs say GPL v3 (see PR #59)
• LICENSE file says MIT
• setup.py says MIT

Obviously, this needs to be consistent.

The choice of license leads us into hotly debated territory that requires lawyers (at least in principle). MDAnalysis itself is licensed under "GPL v2 or any later version". This means that GPL v3 is an allowed license.

@ojeda-e you need to set your license.

The default cookie-cutter code of conduct is not bad but as an MDAnalysis project this project is bound by the MDAnalysis Code of Conduct. The version on the web page is the authoritative version. You can copy https://github.com/MDAnalysis/mdanalysis/blob/develop/CODE_OF_CONDUCT.md into your repository to replace the existing one.

cc @MDAnalysis/gsoc-mentors

@ojeda-e
Because it's relying on the Results class, could you raise an issue as a reminder that MDA should get pinned to >=2.0.0 in setup.py? (I wouldn't do it now, but it'd be a good thing to do before the first release)

Originally posted by @IAlibay in #43 (comment)

The documentation badge currently points to the contents of docs/, which is just a page telling you how to compile the membrane curvature docs: https://www.mdanalysis.org/membrane-curvature/

Could you please change the link to ReadTheDocs, which is currently working documentation? https://membrane-curvature.readthedocs.io/en/latest/

As suggested in #34 (see comment here), break core_fast_leaflet into three different functions to

• get positions for each atom in the atom group for each frame.
• identify the grid cell for each coordinate.
• calculate the average z for the atom group.

Add tests for each function above.

The example provided in the docstrings of base.py displays an incorrect import:
from MDAnalysis.analysis import MembraneCurvature,
that should be
from membrane_curvature.base import MembraneCurvature.
A link that redirects to Usage after the example provided may be useful for the user.

To fix this issue:

• Change import.
• Add a link to redirect to specific examples.

Several lines in core.py and libs/mods.py are not used. As a result, the coverage seems lower.

To do:
In libs/mods.py:

• Clean all lines associated with the previously parsed arguments (lines 106-124)
• Delete all classes. (lines 9-103).

In core.py:

• Clean all lines associated with the previously parsed arguments (lines 53-79)
• Delete all classes (lines 9-45)

As suggested in #13 (comment),
it's better to use explicit imports, which makes the code easier to debug and to read. The preferred form to use is import y and then use y.x.

To do:

• Change location of libs/mods.py
• Use explicit import in core.py.
• Use explicit import in tests/test_mdakit_membcurv.py

Currently, there are two names used in the repo MDAkit_MembCurv and membrane_curvature.
For consistency, change names to membrane_curvature.

Add an ipynb tutorial of a membrane-protein system using the YiiP dataset NhaA dataset.

To include in the tutorial:

• Introduction.
• Description of the system.
  - [ ] Multiframe results.
• Averaged results.
• Surface, mean, Gaussian curvature plots with interpretation.
• Reference.

From PR #40 and comments.
Function derive_surface includes factor = np.float32(n_cells / max_width) in line 30, which is not clear.
Investigate performance of factor = np.float32(n_cells / max_width) vs, factor = n_cells / max_width.

@ojeda-e could you please start putting your thoughts here on how you want the AnalysisBase refactor to go?

In general, AnalysisBase classes will have four methods defined:

• __init__ this receives arguments. Usually an AtomGroup or Universe is provided here to operate over, and user-arguments such as n_cell.
• _prepare this typically sets up containers for the data to be collected in _single_frame, and maybe performs some pre-emptive computation.
• _single_frame this is the bulk of the class: the functionality that you want to operate over every frame of the trajectory. For example, you could be collecting lipid positions.
• _conclude this does the final computation with the data collected from _single_frame. Sometimes it's very short (e.g. https://github.com/MDAnalysis/mdanalysis/blob/64840d5df32ecb73ceadcc022dc4b71738537404/package/MDAnalysis/analysis/rms.py#L864-L870). Sometimes it's where the bulk of the computation is (e.g. https://github.com/MDAnalysis/mdanalysis/blob/64840d5df32ecb73ceadcc022dc4b71738537404/package/MDAnalysis/analysis/pca.py#L315-L322). For example, the calculation of mean and gaussian curvature could go here.
• (optional) post-run methods. If you don't want to compute the mean and gaussian curvature every time the user calls run() but only when they ask for it, you can add post-run methods instead, e.g. https://github.com/MDAnalysis/mdanalysis/blob/64840d5df32ecb73ceadcc022dc4b71738537404/package/MDAnalysis/analysis/pca.py#L431-L476

Unfortunately, the more explicit warning messages introduced in #83 significantly affect the performance of the MembraneCurvature, more particularly in membrane-protein systems, where the messages are overwhelming.
While working on #84, I found a dramatic difference between version 0.0.1 (no explicit messages) and 0.0.2 (explicit messages).
To note, these messages are popping up more often in membrane-protein messages, which is expected, given the fit trans+rot transformation that put lots of atoms out of boundaries.

Possible solutions are:

• Go back to the previous minimal messages in surface.py.
• Set a limit on the number of warning messages that can be triggered.
• Deal with the problem of grid size / wrap coordinates (probably related to #47 )

Could you please add installation instructions? Edit: typically installation instructions are given both in the root repo README.md, and in the documentation on ReadTheDocs. At the moment, as a new user, I'm not sure how to install and use your code. There have not been any releases yet, so people will have to build from source. I usually put my dependencies into another yaml file instead of re-using my test environment, because that has extra packages such as pytest. For examples, yours could look like:

name: membrane-curvature
channels:
  - conda-forge
  - defaults
dependencies:
  - python
  - pip
  - numpy

    # Pip-only installs
  - pip:
    - MDAnalysis==2.0.0b0

If you save that as environment.yml in devtools/conda-envs, building from source would look like:

git clone https://github.com/MDAnalysis/membrane-curvature.git
cd membrane-curvature
conda env create -f devtools/conda-envs/environment.yaml
conda activate membrane-curvature
python setup.py install

Once packages are created for Pypi and conda, those instructions can be added too.

Update ReadMe

• Update documentation badge.
• Add Author.
• Add disclaimer.

Update membrane-only tutorial after #70 is fixed.
Plots of mean curvature should display inverted colors.

With @orbeckst we agreed to have installation via pip for the moment and leave the conda option for a future release.

So once you do your first release I assume the aim is that this will become available via pip / conda (pinging @lilyminium @orbeckst @fiona-naughton ) - I guess these instructions can be updated after the fact?

Originally posted by @IAlibay in #65 (comment)

As pointed out by @hmacdope here, there is no information about curvature units.

To solve this issue:

• Add information of curvature units to docstrings in curvature.py

A blog post on this matter may be helpful too.

To test mapping (grid) using pytest.mark.parametrize as suggested in PR #30 , refactor def_all_beads to

• replace MDtraj by MDAnalysis.
• Pass arguments as np.array instead of topologies.
  Also add:
• Test function 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.

Once you have added your initial code (#7) you should write tests that produce results that you know to be correct.

Once you start refactoring your code, the tests will remain the same (at least the reference values ("fixtures") that they check against). This will provide you the confidence that your code changes do not introduce bugs. (In truth, you'll likely discover bugs in your old code and new bugs will creep in but still, your initial tests will be the foundation on which you're doing your work).

Hello,

For a particular system, I am getting the following error

MDAnalysis.MDAKit.membrane_curvature: WARNING  Atom coordinates exceed size of grid and element (25,13) can't be assigned. Maximum (x,y) coordinates must be < (137.42520141601562, 137.42520141601562). Skipping atom.

What does this exactly mean? I am not able to understand why this warning is raised because the atoms are selected from the PDB files specified so no atom should really be outside of the grid.

In surface.py, the messages "Atom outside grid boundaries" in lines 92 and 100 are not logged.
The warning doesn't provide further or more detailed information about the atom, i.e. if it's because of negative coordinates, etc.

Thanks @orbeckst for spotting it (see review in notebook)

To fix this issue:

• Add logger.warn(msg).
• Edit message to provide more details.
• Add respective tests.

Current docs are incomplete and lack API docs.

To fix this issue:

• Add API docs using reStructured Text, docstrings and autosummary. (Include MembraneCurvature class and functions in curvature.py and surface.py)
• Update index.rst
• Update getting_started.rst

@lilyminium hopefully I am not missing anything here at the moment. :)

Fix the links in the badges in the README so that they all display properly.

After adding PBC/coordinate wrapping and while testing the exception in line 76-77 of surface.py, I noticed IndexError is not raised. Issue identified by the message
AssertionError: Regex pattern 'index is out of bounds' does not match 'index 3 is out of bounds for axis 0 with size 3'.

If the exception is not raised, should be removed. However, there must be some sort of IndexError or ValueError raising to notify the user there is a potential loss of information by lack of sampling.

To fix this issue:

• Figure out if the exception is needed. If not, provide an alternative.
• Add tests to raise the exception if included.

A documentation badge is added in #4, but the link it points to https://www.mdanalysis.org/membrane-curvature/ doesn't actually exist yet. I've set up the ReadTheDocs webhook at https://membrane-curvature.readthedocs.io/en/latest/ for now. We have a couple options for the final docs:

• pointing RTD to something like membrane-curvature.mdanalysis.org (I don't think RTD will accept https://www.mdanalysis.org/membrane-curvature/)
• Setting up a gh-pages branch like https://github.com/MDAnalysis/pmda , which will automatically serve docs at https://www.mdanalysis.org/membrane-curvature/ (as is done for the main repository and user guide documentation)

Any opinions @ojeda-e @orbeckst @fiona-naughton @IAlibay ?

To make it more obvious what this repo is about, I suggest to

• update the description of the repo (the About setting on the repo front page)
• add a link to your blog (also About(

To test coordinate mapping and average of z values in grid using pytest.mark.parametrize as suggested in PR #30, refactor core_fast_leaflet and core_fast to

• replace MDtraj by MDAnalysis.
• Pass arguments as np.array / tuples instead of topologies.

Also, add:

• Test function 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.
• Test function using toy model in a small grid of 16 lipids in a 4x4 grid, with x values of 0, 1, 2,3 and y values of 0, 1, 2,3. Use different z values of beads for this test.

From merged PR #33 the following changes are pending:

• Add a fail test with negative coordinates in grid_map.
• Combine the 9-grid and the 25-grid into one function with pytest.parametrize.
• Add a test where the output of grid_map is different from the input dummy_coord.
• In line 187, check test_mapper. Suggested change: assert test_mapper(dummy_coord) == grid_map(dummy_coord, factor) for assert grid_map(dummy_coord, factor) == dummy_coord.

Context:
The figures included in docs use either contourf or imshow. In both cases, the resulting surface/curvature array is plotted as (M, N) array-like.

From the plt.contourf docs:

Parameters
-----------

X, Y array-like, optional: 
    The coordinates of the values in Z.

    X and Y must both be 2D with the same shape as Z (e.g. created via numpy.meshgrid), 
    or they must both be 1-D such that len(X) == N is the number of columns in Z and 
    len(Y) == M is the number of rows in Z.

    If not given, they are assumed to be integer indices, i.e. X = range(N), Y = range(M).

Similar story with plt.imshow. Hence, the resulting surface/curvature array should be transposed to generate the correct plot.

To illustrate the issue, here below is an example using plt.imshow from the Membrane Curvature Visualization page

Current plot: 😞

Expected plot: ☺️

To fix this issue, update all the figures in the visualization and tutorial pages.

As #13 notes, while it introduces some initial tests for the final result of each curvature function, additional and more specific intermediate tests (e.g. for getting the z-coordinates from a particular trajectory) will be added. In order to keep an eye on progress with the automated project tracker (https://github.com/MDAnalysis/membrane-curvature/projects/1), perhaps you could create some more issues for what you plan to do in the coming weeks, @ojeda-e?

For example, instead of "write tests for MDTraj code", an issue could be "write tests for the core_fast_* functions" (and add it to the MDTraj milestone). You could also look further ahead and start planning the structure of the MDAnalysis re-factor -- which functions you want to do first, etc.

I think the project will automatically track created issues and PR's, so the only manual work is adding labels and/or milestones. Edit: sorry, you will have to manually add issues and PR's to the project tracker and milestones. This is quite a lot of project management so let us know if it gets too redundant and you prefer one or the other -- I am new to GitHub project management too!

I do like milestones because they have deadlines, so you can see how you're going in a particular area time-wise. They're usually overarching goals that have many issues and PR's attached (e.g. MDAnalysis uses them for releases). I would guess that "MDAnalysis refactor" would be the next big one. The milestones tab can be a bit hard to find so I will just it here: https://github.com/MDAnalysis/membrane-curvature/milestones?direction=asc&sort=due_date

The initial version of the code uses the mdtraj library.
Next step is to replace all functions from mdtraj by MDAnalysis modules and adjust the code wherever is necessary.

Changes required in

• core.py
• /lib/mods.py.

This repository uses GitHub actions to run tests. To check that the tests added for issue #10 work, the CI suite will need to be set up. The workflow is in CI.yaml here: https://github.com/MDAnalysis/membrane-curvature/blob/main/.github/workflows/CI.yaml

In case you're not familiar with GH actions, I've annotated the file with some notes:

name: CI

Pretty self-explanatory, it's the name that shows up in the Actions tab.

on:
  # GitHub has started calling new repo's first branch "main" https://github.com/github/renaming
  # Existing codes likely still have "master" as the primary branch
  # Both are tracked here to keep legacy and new codes working
  push:
    branches:
      - "master"
      - "main"
  pull_request:
    branches:
      - "master"
      - "main"
  schedule:
    # Nightly tests run on master by default:
    #   Scheduled workflows run on the latest commit on the default or base branch.
    #   (from https://help.github.com/en/actions/reference/events-that-trigger-workflows#scheduled-events-schedule)
    - cron: "0 0 * * *"

The on section specifies when the CI suite runs. Here it runs whenever you push to branches called "master" or "main". It also runs when a pull request is made to "master" or "main", and then again nightly on "main". You could switch this one off if you want to save some energy.

jobs:
  test:
    name: Test on ${{ matrix.os }}, Python ${{ matrix.python-version }}
    runs-on: ${{ matrix.os }}
    strategy:
      matrix:
        os: [macOS-latest, ubuntu-latest, windows-latest]
        python-version: [3.7, 3.8, 3.9]

    steps:
    - uses: actions/checkout@v1

    - name: Additional info about the build
      shell: bash
      run: |
        uname -a
        df -h
        ulimit -a


    # More info on options: https://github.com/conda-incubator/setup-miniconda
    - uses: conda-incubator/setup-miniconda@v2
      with:
        python-version: ${{ matrix.python-version }}
        environment-file: devtools/conda-envs/test_env.yaml

        channels: conda-forge,defaults

        activate-environment: test
        auto-update-conda: false
        auto-activate-base: false
        show-channel-urls: true

    - name: Install package

      # conda setup requires this special shell
      shell: bash -l {0}
      run: |
        python -m pip install . --no-deps
        conda list


    - name: Run tests

      # conda setup requires this special shell
      shell: bash -l {0}

      run: |
        pytest -v --cov=mdakit_membcurv --cov-report=xml --color=yes mdakit_membcurv/tests/

    - name: CodeCov
      uses: codecov/codecov-action@v1
      with:
        file: ./coverage.xml
        flags: unittests
        name: codecov-${{ matrix.os }}-py${{ matrix.python-version }}

The jobs section specifies which jobs are actually run. The first test job is called Test on ${{ matrix.os }}, Python ${{ matrix.python-version }}, using variable substitution to make names like Test on macOS-latest, Python 3.7. The runs-on tells GH which machine or operating system to run the test on. The strategy and matrix tells GH which potential options to use for matrix.python-version and matrix.os.

The next part of the job specification, steps, is a list of steps that starts from - uses: actions/checkout@v1 and encompasses the rest of the file. In the yaml format, - denotes an item in a list and : denotes a key-value pair like a dictionary, so each item in the steps list is a dictionary that can have keys and values in any order. Therefore it's a bit random which key is shown for each step, although defining the name first is probably best for readability.

1. - uses: actions/checkout@v1
   This checks out the current branch of the repository, so tests are being run with your updated code.

2. - name: Additional info about the build
   In this step the commands uname -a; df -h ; ulimit -a are run in a bash shell to tell us more about the machine. This is very helpful for troubleshooting if tests are failing for an unexpected reason.

3. - uses: conda-incubator/setup-miniconda@v2
   This installs a conda environment from devtools/conda-envs/test_env.yaml (https://github.com/MDAnalysis/membrane-curvature/blob/main/devtools/conda-envs/test_env.yaml) . When you start using packages like numpy, you'll need to add them to that conda environment so that they'll get installed in the test environment.

4. - name: Install package
   This installs the membrane-curvature package from setup.py.

5. - name: Run tests
   This actually runs the tests in the test suite!

6. - name: CodeCov
   This uses codecov to check how much of the code is covered by tests.

Most of the suite has already been set up in this file. However there are some changes you can make:

• Update https://github.com/MDAnalysis/membrane-curvature/blob/main/devtools/conda-envs/test_env.yaml to include the packages you'll need
• Add fail-fast: false to the strategy section, like in the MDAnalysis repo: https://github.com/MDAnalysis/mdanalysis/blob/a6d4ffc26375a44d4aaa53de88d193b9e1e645e9/.github/workflows/gh-ci.yaml#L35
• Add concurrency groups, like in the MDAnalysis repo: https://github.com/MDAnalysis/mdanalysis/blob/a6d4ffc26375a44d4aaa53de88d193b9e1e645e9/.github/workflows/gh-ci.yaml#L12-L17

(The MDAnalysis GH actions file is a lot messier than this one, but luckily this repo doesn't need all that complexity).

The fail-fast addition means that even if one test run fails, the others don't get cancelled immediately. The concurrency addition means that if you push to a branch before the tests have finished running, those running tests get cancelled in favour of the updated code. Running the tests does require electricity so this will save a 🌴 or two.

To help with organizing issues

• create a label "continuous integration"
• label issue #11 with this label (in addition to the "testing" label that I had added)

Check the boxes and close this issue when you're done.

Create more labels that you think might be helpful for you and us. Tag your issues/PRs correspondingly.

Installation instructions can be found in the README file only. They should appear in RTD too.

To fix this issue:

• Add installation instructions to index in RTD.

Now that MDAKits are live to roll we should work towards registering membrane-curvature as an MDAKit!

See the blog post for more info.

AFAIK membrane-curvature already meets all the requirements listed in the white paper.

Docs need tutorials with more complex examples than the one provided on the Usage page and with detailed steps to calculate membrane curvature.

To fix this issue:

• Add tutorials page.
• Add notebook 1, with membrane-only system.
  Edit: Each notebook will be a separate issue.
  - [ ] Add notebook 2, with membrane-protein system, protein with posres.
  - [ ] Add notebook 3, with membrane-protein system, protein with no posres.

Related to #69: I think the sign of the mean curvature is reversed here compared convention.

Conventionally I think we use the upward-pointing Z-axis as the membrane normal. With that, you can use this definition of the mean curvature (as you do). But I think you've added an extra negative. Should that be there?

Write test for core_fast function in mods.py. This will cover lines 233-241.

This issue will help to close #17

Write test for def_all_beads in mods.py to cover lines 233-241.

This issue will help to close #17

When calculating get_z_surface undefined values in bins inside the embedded element (i.e. protein) may arise since no z coordinates populate bins in the grid. Such undefined values spread in the array during the calculation of curvature.

One possible solution is to interpolate the bins occupied by the protein. I would avoid using pandas although that approach seems the most straightforward. Check np.interpolate.

The code to transform xy coordinates to grid cells is important and modular enough that you could keep it as a function in the AnalysisBase refactor. Therefore, it should get tested extensively to make sure that it is correct, especially as the regression tests for the mean and gaussian curvature in #13 depend on z_ref coordinates that were presumably generated this way. Writing the test could be done before, or in parallel, with starting to outline the AnalysisBase re-factor.

Instead of testing the generation of z_ref on the GRO file you have uploaded, a simpler test that is more easy to humanly verify would be a small toy system of fake coordinates (maybe 5-12 coordinates per axis for a suitable corresponding number of cells). You could use pytest.mark.parametrize to test multiple inputs (or you could have separate tests for each one if that's more your style!). You might need to have at least two tests though, one for tests that currently pass and one for tests that are expected to fail (marked with XFAIL, e.g. those with PBC) These inputs could include:

• negative coordinates
• coordinates right on the bounds of each cell
• a square input (e.g 8 x 8)
• a rectangular input (e.g. 8 x 12)
• coordinates higher than the box size (e.g. for unwrapped trajectories)
• the scenario of z-coordinates on the bottom of the box and requiring PBC

For example, I would check that a system of [(-1, 0), (0, 0), (1, 0), (2, 0), (3, 0)] gives a grid with the (-1, 0) coordinate last and only one y-row. This smaller system is much easier to understand than a real membrane, and defines expectations such as PBC.

The Algorithm and Usage pages are incomplete in current docs

To fix this issue, update:

• Algorithm page.
• Usage page.
• Visualization page: include plt.contourf plots.
• Visualization page: include plt.imshow plots.

Periodic Boundary Conditions (PBC) are not applied when mapping coordinates to grid (grid_map).

To fix this issue:

• Add PBC conditions to map coordinates.
• Add test using dummy coordinates to map values that exceed by defect or excess the boundaries of the grid.

For example:

• System with 9 beads with coordinates of x -1, 0, 1 and y -1, 0 ,1
• System with 16 beads with coordinates of x -2, -1, 0, 1, and y -2, -1, 0 ,1, 2.

AnalysisBase in PR #41 does not consider NPT fluctuations.
Density analysis deals with a similar problem. https://docs.mdanalysis.org/1.1.1/documentation_pages/analysis/density.html

How do you deal with NPT-led box fluctuations?

Originally posted by @IAlibay in #43 (comment)