New analysis testing utilities#1068
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1068 +/- ##
=======================================
Coverage 81.56% 81.56%
=======================================
Files 140 140
Lines 15452 15452
=======================================
Hits 12604 12604
Misses 2848 2848 Continue to review full report at Codecov.
|
|
This is ready for review and comment. I will leave it open for at least 48 hours, merging no earlier than Tue 14 Sep 20:00 GMT (16:00 my local). Coverage doesn't show, since the For what it's worth, I'm already playing with this in a branch where I've mixed this stuff and #1026. Writing tests for #1026 is so easy now. |
sroet
left a comment
There was a problem hiding this comment.
Looks great,
2 critical points:
- I am not convinced "make_trajectory" does what it is supposed to,
- I would like to have access to the
final_statein themake_tis_trajectory
Also pointed out the issues my development environment was complaining about, feel free to ignore those
| @pytest.fixture | ||
| def tis_network(): | ||
| paths.InterfaceSet._reset() | ||
| cv = paths.FunctionCV("x", lambda s: s.xyz[0][0]) |
There was a problem hiding this comment.
I am not sure about "import order", but would these fixture work for functions that require monkey (un)patching?
There was a problem hiding this comment.
The CVs used here are not compatible with SimStore, so this should not be used if the intent is to also test SimStore storage.
Of course, that will change in 2.0 when paths.FunctionCV is paths.experimental.storage.CollectiveVariable.
| @pytest.mark.parametrize('bounds', [(-0.1, 1.0), (-0.1, 0.5)]) | ||
| def test_make_trajectory(bounds): | ||
| lower, upper = bounds | ||
| traj = make_trajectory(lower, upper) | ||
| xvals = traj.xyz[:,0,0] | ||
| assert upper <= max(xvals) < upper + 0.1 | ||
| assert lower <= min(xvals) < lower + 0.1 | ||
| expected_len = round((upper - lower) / 0.1) + 1 | ||
| assert len(traj) == expected_len |
There was a problem hiding this comment.
Just an FYI: be careful about using round it uses (the correct) bankers' rounding, which might bite you if you do not expect it. (for the current values this test is fine, but for `bounds=(-0.55, 0) it is not)
>>> round(2.5)
2
>>> round(1.5)
2Also, this test fails if upper is not divisible by 0.1 (bounds=(0, 0.45) fails as does bounds=(0, 0.55)
| xvals = np.arange(lower, upper + 0.01, 0.1) + np.random.random() * 0.1 | ||
| return make_1d_traj(xvals) |
There was a problem hiding this comment.
There is no guarantee that upper is crossed here
say lower=0, upper=0.09 then the xvals become [0+epsilon], which only reaches 0.09 10% of the time.
|
|
||
| def make_tis_trajectory(cv_max, lower_bound=-0.1): | ||
| """Make a TIS trajectory with a given maximum x value. | ||
|
|
||
| """ | ||
| increasing = make_trajectory(lower_bound, cv_max) | ||
| if cv_max >= 1.0: | ||
| return increasing | ||
| else: | ||
| decreasing = increasing.reversed[1:] | ||
| return increasing + decreasing |
There was a problem hiding this comment.
The cutoff between switching from increasing to both should probably be user configurable, also 2 blank lines expected and only 1 found
| def make_tis_trajectory(cv_max, lower_bound=-0.1): | |
| """Make a TIS trajectory with a given maximum x value. | |
| """ | |
| increasing = make_trajectory(lower_bound, cv_max) | |
| if cv_max >= 1.0: | |
| return increasing | |
| else: | |
| decreasing = increasing.reversed[1:] | |
| return increasing + decreasing | |
| def make_tis_trajectory(cv_max, lower_bound=-0.1, final_state=1.0): | |
| """Make a TIS trajectory with a given maximum x value. | |
| """ | |
| increasing = make_trajectory(lower_bound, cv_max) | |
| if cv_max >= final_state: | |
| return increasing | |
| else: | |
| decreasing = increasing.reversed[1:] | |
| return increasing + decreasing |
|
Sorry that this has been on pause for so long. I'll go ahead and take the PEP8 improvements. However, some of the other comments from review made it clear to me that my code was not communicating its purpose well enough, because that purpose was not obvious to @sroet. I've been trying to figure out what the best way to make the purpose more clear would be -- but rather than try to predict was others will think, it's better to just ask! Fundamental ideas that I don't think have been communicated clearly enough here are mainly about the
Here are a few options I've considered on how to make these things clearer to others reading the code. If you have options, please suggest! Regarding point 1:
Regarding point 2:
These are the things I thought of; again, I'm very open to other options. The code in this PR is very much intended for re-use by contributors (please let me make it easier for you to write tests!), so it is extremely important that it is easily understood. I'm somewhat leaning toward the more complicated solution in both cases (especially point 1, where it enables me to encapsulate the |
Co-authored-by: Sander Roet <sanderroet@hotmail.com>
|
About point 1: I agree that if things are only to be used together, they should be grouped together in 1 object (instance or class) About point 2: |
This links the make_tis_trajectory method to a fixture class that depends on the details of the setup. In this way, there's less of an implicit dependence on a certain kind of setup. Also switches it so that the input values for trajectories here are based on integers instead of floats, which should avoid confusion. Probably still need significant docs here, but the tests seem to pass locally, so that's good.
|
I ended up doing some pretty significant changes since the last review, so this is probably worth a complete re-review, as opposed to just reviewing changes since last review. I re-organized according to the discussion in #1068 (comment) and following. Some of the things that are currently in Here's what's now included in this PR:
|
sroet
left a comment
There was a problem hiding this comment.
Some extra imports, and some possible corrections on the docs. LGTM otherwise
Co-authored-by: Sander Roet <sanderroet@hotmail.com>
sroet
left a comment
There was a problem hiding this comment.
LGTM, feel free to merge on green
Writing tests for analysis code is hard. I always tell people that the input to most analysis methods should be a list of steps. But how do you mock up a sequence of steps?
This PR adds some utilities in
tests/analysis/utils/to help create mock simulation results. These utilities have their own test in that same directory.This was developed because the headache of writing such tests has been blocking both #1026 and #1048 for months. I decided to make a separate task out of creating tools to solve that generic problem.
The approach used here is to use as much of the internal OPS machinery as possible, instead of just reproducing the data it generates (as was done in the OPSPiggybacker). Here, we make use of
unittest.patchto inject the data that OPS would have created via simulation. This means that if internal structure of movers (or ofDetailsthey return) changes, this code should automatically use that new behavior.The scope of this PR is only aiming to support one-way shooting moves, path reversal moves, and repex moves, as well as wrapping as with an
OrganizeByMoveGroupglobal strategy. Support for other movers will be added as needed in the future.