New iter_generate for OPS 2.0#1042
New iter_generate for OPS 2.0#1042dwhswenson wants to merge 5 commits intoopenpathsampling:dev-2.0from
iter_generate for OPS 2.0#1042Conversation
There are still some problems with a few tests -- integration with EngineMover seems a bit off. I need to compare to the old behavior to see what has changed.
|
There's a weird thing in the unit tests for the minus mover. I changed some behavior there, but I honestly don't understand why the previous behavior was occurring. Essentially, the minus move consists of the replica exchange stage followed by an extension stage. There's a test for the case that the minus move fails because the extension hits max length, which uses this code. For some reason, the old test checked that the extension trajectory was of a length longer than the engine's maximum. I don't know why that would be. The test itself also modified the engine's error handling (setting Relevant code is in tests for both minus mover and single replica minus mover: openpathsampling/openpathsampling/tests/test_pathmover.py Lines 1355 to 1358 in c67edf7 openpathsampling/openpathsampling/tests/test_pathmover.py Lines 1517 to 1522 in c67edf7 |
Codecov Report
@@ Coverage Diff @@
## dev-2.0 #1042 +/- ##
========================================
Coverage 81.54% 81.54%
========================================
Files 140 140
Lines 15399 15400 +1
========================================
+ Hits 12557 12558 +1
Misses 2842 2842
Continue to review full report at Codecov.
|
|
Not sure why we're getting @codecov updates on a branch where the CI doesn't report coverage... I wouldn't trust the data. |
Resolves #810.
One of the things I want to do in OPS 2.0 is simplify code in API-breaking ways, when the API isn't something that is widely used. One case of this is
DynamicsEngine.iter_generate. The existingiter_generatehas a huge complexity rating (McCabe cyclomatic complexity of 46; CodeClimate "cognitive complexity" of 92). This comes because it tries to implement a bunch of user-facing options for error handling into one catch-all function, instead of letting the client code handle the errors. This is all the stuff about how to handle NaNs or max-length error (whether to restart/retry/etc.)In path sampling, many of those options would be just wrong. A trajectory that generates a NaN is a rejected trajectory. Retrying will bias the sampling. In addition, when I tried to write tests for it, it became clear that some of that functionality isn't even implemented correctly.
So this PR rewrites
iter_generatewithout the extra error handling functionality. That error handling should be done in the calling code. We raise errors that should make it clear to calling code what the problem is; special treatments in case of certain error types can be done by calling code (this is never something that should be done on a per-engine level, but something that should be done on a situational basis).There are two places where engine writers can change the error handling:
engine.is_valid_snapshot(): Check whether the created snapshot is valid. Generally will just return a bool. If you want to use the default error behavior (raise anEngineError), this is all you need to change.engine.snapshot_error_handler(): If you want to modify the error in some way, or if there are some errors that you can recover from, do that here. If you want an error to be raised, then return the error. If you can recover from the error and create a valid snapshot, then return the valid snapshot.For most users, this makes no difference. It means that under the hood we're using a different function. The API break is the drop of engine-level support for error handling, and a small change in the call signature of
iter_generate(removing theintervalsparameter). I don't think anyone will miss these things.iter_generateiter_generatefor all testsiter_generate; full coverage