Closed ojeda-e closed 3 years ago
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:
I might be missing something, but could you please explain how the coordinate wrapping treats PBC? As far as I can tell you're still not treating the boundary rows and columns of the surface grid?
I'm also not convinced that you can validly wrap atom coordinates when they have been rotated and translated as I believe you preprocess your trajectories to do. Perhaps you could instead get the translation, or translation + rotation matrix (to center on the protein if any) for each frame, and use that to transform your surface?
This is indirectly related to @orbeckst's project 5 of GSoC (https://github.com/MDAnalysis/mdanalysis/wiki/Project-Ideas-2021#project-5-general-unit-cell-representation), by the way. I'm just pointing that out of interest and as more motivation for the project to go forward rather than suggesting you implement it (although you are welcome to, of course!) Just storing the matrix for each frame of this analysis would be a great starting point.
Edit: sorry if I'm a bit short here, just trying to catch a few bars of signal when I can find it -- which is less than I originally expected
I pushed changes to use wrap
instead of PBC. I didn't push changes for the keyword that will discriminate between raw and processed trajectories, as we discussed in this comment.
To clarify, should I add such changes in this PR as well @lilyminium @orbeckst @IAlibay
I think it is good enough with wrap
. Instead, I can have clear documentation that says "wrap=True
only if your Universe has a raw trajectory." or similar. We don't really need an extra keyword to discriminate the systems.
@orbeckst @lilyminium @IAlibay @richardjgowers do I have a green light here? Thanks.
@ojeda-e looks good but I'd clarify what wrapping is doing here. PBC treatment is confusing as it means one of two very different things
I would also add some documentation explaining when you should use
wrap=True
and explain in words what it achieves.
Thanks, @orbeckst, initially I was planning to add the explanation of when to use wrap=True
in the Usage (Issue #58)
Now that you point it out I'll add a brief description here in base.py
and a more detailed one in Usage.
Can I have an approval here, please? @richardjgowers @orbeckst @lilyminium
I discover a "little" bug in my test_po4_small.gro
(see comment). Given that the size of the box in z
was 30 A, ag.wrap
was returning z=0
(and therefore everything was zero, surface, mean, Gaussian). I also deleted the trajectory test for now, as I mentioned in this comment. I don't think is strictly necessary, but if you think is best to add the traj, I will add it. I hope we all agree this change doesn't represent any harm or has a negative impact in the tests.
Thanks to @lilyminium for asking me to add tests with wrap=False
and wrap=True
because otherwise, I'd have never seen that bug. :)
I added two fixtures for universes:
universe_dummy_wrap
with atoms out of bounds in x
,universe_dummy_wrap_xy
with atoms out of bounds in x
and y
.Intentionally, I made the two fixtures match with the derived z_surface
when wrap=True
is applied. Then, the third fixture added dummy_surface
. For each one of the universes with atoms out of bounds, I added a test for
For each test that involves curvature, I calculated mean_curvature
and gaussian_curvature
directly from dummy_surface
.
I also discover another bug in surface.py
(see comment). But fixed it with an if statement. Hope you are ok with this change as well.
Thanks again @lilyminium
Thank you for adding the new tests @ojeda-e!
The pushed changes include:
[x] Corrected indexes in diagram:
# +-----------+ +-----------+
# | | 8 | 9 | 7 | 7 | 8 | 9 |
# +-----------+ +-----------+
# 6 | 4 | 5 | | ---> | 4 | 5 | 6 |
# +-----------+ +-----------+
# | | 2 | 3 | 1 | 1 | 2 | 3 |
# +-----------+ +-----------+
#
[x] Added fixtures that returns membranecurvature.run() for each universe with unwrapped coordinates:
curvature_unwrapped_universe(self, universe_dummy_wrap)
curvature_unwrapped_universe_xy(self, universe_dummy_wrap_xy)
[x] Removed previous tests for x and y coordinates as suggested, and added an extra one with negative z coordinates with 4 bins.
[x] In surface.py
, prints were replaced by warnings as suggested. However I have a question here:
base.py
. But for the warning in surface.py
, I am not sure I should add a logger. Would it make sense to add a logger there?Thanks @lilyminium
Thanks for checking this @lilyminium! This was a very buggy PR 🐛 but glad it's all fixed now! :D
In this PR I added PBC conditions. Changes in this PR fix #36 and possibly #22
self.ag.wrap()
added to__init__
and_single_frame
.results.z_surface
andresults.mean
were updated.xfail(reason="PBC conditions not applied.")
(See line 276 istests_membrane_curvature.py
.@orbeckst @lilyminium @IAlibay are there any other relevant tests I am missing here? Thanks