ENH: hole trajectory analysis: can use dynamic cpoint#1814
ENH: hole trajectory analysis: can use dynamic cpoint#1814
Conversation
|
Not sure why the full py 3.6 test passes (I assume the HOLE tests are not skipped because HOLE is installed) but the numpy version 1.10.4 test fails in one of the HOLE tests. ... TODO. |
|
(I'm just restarting the job, just to check that it is reproducible.) |
package/MDAnalysis/analysis/hole.py
Outdated
| logger.info("HOLE analysis frame %4d (orderparameter %g)", ts.frame, q) | ||
| if self._dynamic_cpoint: | ||
| hole_kw['cpoint'] = self.cpoint.center_of_geometry() | ||
| logger.info("New dynamic CPOINT %r for frame %r", hole_kw['cpoint'], ts.frame) |
There was a problem hiding this comment.
This has the potential to be super verbose. Wouldn't it be more convenient to save the cpoint in an array instead?
There was a problem hiding this comment.
By verbose you mean "giving output every frame"? Yes, I'll have to check what else is printed. In practice, I would not run this on 1000s of frames because it is pretty slow.
Wouldn't it be more convenient to save the cpoint in an array instead?
You mean as an attribute of the class for further investigation later? That's a good idea.
There was a problem hiding this comment.
For right now, I changed all per-frame logging to logger.debug. Should use progressmeter for some...
| p = profiles_values[0] | ||
|
|
||
| assert len(p) == 425, "wrong number of points in HOLE profile" | ||
| assert_equal(len(p), 425, err_msg="wrong number of points in HOLE profile") |
There was a problem hiding this comment.
Why revert back to assert_equal? Didn't we agree that with pytest we should use raw asserts expect when comparing arrays?
There was a problem hiding this comment.
I thought we said "use assert_equal() but not assert_array_equal()" – I don't think that bare assert can properly compare arrays.
There was a problem hiding this comment.
... but yeah, in this case this should be assert.
There was a problem hiding this comment.
(I think I used it for making clear where the error message goes... but simple assert should also work there.)
I am also replacing assert_array_* --> assert_*.
| assert len(open(filename).readlines()) == 6504, "HOLE VMD surface file is incomplete" | ||
| with open(filename) as f: | ||
| nlines = len(f.readlines()) | ||
| assert_equal(nlines, 6504, |
There was a problem hiding this comment.
Same as above: why assert_equal?
There was a problem hiding this comment.
Can't remember why I changed it. Should be reverted. Good catch.
| class TestHOLEtraj_dynamic_cpoint(_TestHOLEtraj): | ||
|
|
||
| @pytest.fixture() | ||
| def reference(self): |
There was a problem hiding this comment.
This is the same reference as above. Does it have to be repeated in the class?
There was a problem hiding this comment.
Probably not, I might well only have been starting to get the hang of using pytest fixtures.
There was a problem hiding this comment.
Actually, it is not the same reference. The values are subtly different for "mean HOLE rxncoord", "minimum radius", and "min_radius" because the CPOINT parameter was varied. The test trajectory does not move very much so it does not make a big difference.
I'll leave it as it is.
package/MDAnalysis/analysis/hole.py
Outdated
| description). | ||
|
|
||
|
|
||
| .. versionchanged:: 0.17.1 |
There was a problem hiding this comment.
18.1 (which I'll rename to 19.0 once we push)
The hole.HOLEtraj kwarg cpoint can now also a AtomGroup. In this case, the cpoint is recalculated for each frame as the COG of the ag, which helps with proteins that undergo conformational changes. Basic tests were added.
|
rebased against develop and force-pushed |
|
The only test that fails is the numpy 1.10.4 one in
I honestly have no idea yet why/where the numpy version makes a difference but it is disturbing and I don't want to brush this off. Any suggestions welcome. |
|
I can reproduce the test failures locally in a numpy 1.10.4 + python 3.5 environment (minor aside: conda-forge gsd is not compatible with this environment so needs to be pip-installed). When I find a bit more time I can try to dig into the differences – but if anyone has any hints why this could produce different results then please let me know. All the actual computation is supposedly done by HOLE itself. |
|
Would need to be rewritten for the new hole2 module #2523. |
4 participants
Changes made in this Pull Request:
The hole.HOLEtraj kwarg cpoint can now also a AtomGroup. In this case, the cpoint is recalculated for each frame as the COG of the ag, which helps with proteins that undergo conformational changes.
Basic tests were added.
PR Checklist