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

Surface Interpolation Function [Fixes Issue45] #52

Closed ojeda-e closed 8 months ago

ojeda-e commented 3 years ago

This is a starting point to solve #45

Changes in this PR:

As suggested by @orbeckst in this comment, the function here included uses NumPy. Thanks for encouraging numpy!

Surfaces included in tests are dummy_arrays with the following characteristics:

Probably I'll be asked to add more tests, but this is ok to start. Probably @lilyminium will give me lots of ideas. 😅

Thanks.

pep8speaks commented 3 years ago

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

Line 7:88: W291 trailing whitespace

Comment last updated at 2021-08-07 22:39:35 UTC
codecov[bot] commented 3 years ago

Codecov Report

Merging #52 (7d7f4ca) into main (8a52295) will not change coverage. The diff coverage is 100.00%.

ojeda-e commented 3 years ago

The pushed changes include:

I know base.py could be written better, but maybe we can leave that improvement for later and work on the essentials for now if that's ok. Hopefully I am not missing relevant things here. Thanks!

ojeda-e commented 3 years ago

@lilyminium sorry, it's my fault because I made a bit of a mess here. To clarify. def test_mean_curvature_small passes with assert_allclose. I don't know why I ended up adding rtol it should go. (I added the comment here)

The problem is in def test_gaussian_curvature_all. The test passes with rtol=4 or atol=1e-7. From the docs it says allclose "... compares the difference between actual and desired to atol + rtol * abs(desired)." So probably better have assert_allclose(k, k_test, atol=1e-7) ?

lilyminium commented 3 years ago

So probably better have assert_allclose(k, k_test, atol=1e-7) ?

I think so. Even if you know yourself that the 400% difference is small because the real value is small, it communicates to future users that you expect the degree of difference to be around 0.0000001.