Compute gradients with respect to grid spacing

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

Compute gradients with respect to grid spacing #116

Closed ctlee closed 1 week ago

ctlee commented 9 months ago

Description

The calculation of the gradients of the height field should respect the grid spacing. This PR adds support for variable grid spacing. It also adds some tests against parametric surfaces.

fixes #115

Todos

Notable points that this PR has either accomplished or will accomplish.

[x] Make np.gradient respect grid spacing
[x] Add test to compare computed mean and gaussian curvature against analytical formula for hemisphere
[x] Add test to compare computed mean and gaussian curvature against analytical formula for monkey saddle
[x] Fix unit tests to check against curvature calculations which respect grid spacing.

Questions

N/A

Status

[x] Ready to go

pep8speaks commented 9 months ago

Hello @ctlee! 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 138:65: W291 trailing whitespace

In the file membrane_curvature/tests/test_membrane_curvature.py:

Line 135:77: W291 trailing whitespace Line 136:76: W291 trailing whitespace Line 145:16: W291 trailing whitespace Line 168:80: E501 line too long (99 > 79 characters) Line 173:80: E501 line too long (85 > 79 characters) Line 189:16: W291 trailing whitespace Line 193:31: W291 trailing whitespace Line 225:80: E501 line too long (81 > 79 characters) Line 502:80: E501 line too long (119 > 79 characters) Line 507:80: E501 line too long (119 > 79 characters) Line 513:62: W291 trailing whitespace Line 629:80: E501 line too long (81 > 79 characters) Line 630:80: E501 line too long (91 > 79 characters) Line 631:80: E501 line too long (81 > 79 characters)

Comment last updated at 2024-08-20 10:36:04 UTC

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (44d136b) to head (35ceed9). Report is 9 commits behind head on main.

Additional details and impacted files

IAlibay commented 8 months ago

@ctlee Apologies for the very late response here - unfortunately membrane curvature doesn't have an active maintainer currently so there haven't been anyone to shephred pull requests.

We will try to get someone to review your code as soon as possible. Pinging @MDAnalysis/coredevs here in case someone might want to take on an initial review to help this process along.

IAlibay commented 8 months ago

Updating against main should help get CI working properly here (since we've now merged #112 )

hmacdope commented 8 months ago

Sounds reasonble @ctlee, I am happy to give my approval on a purely code basis, but from a scientific basis I will have to take your assertions about the correctness of the new implementation relative to the old one on trust. I will tag @ojeda-e as the principal developer as she may have a view here. I will leave my view blocking for now pending waiting for feedback. :)

ctlee commented 7 months ago

Following up on this as new versions appear to be released! Folks will get incorrect curvature values if their grid spacing isn't 1. Can verify this issue in the current implementation by comparing with the parametric surface tests added in this PR.

An alternative approach is to add a check that the dx, dy is 1 and to throw an error otherwise. @ojeda-e

hmacdope commented 2 months ago

@ojeda-e are we happy with this PR as is? I would be happy to go ahead, but thought would check with you?