The engine.iter_generate method has a bunch of error handling, including things to manage retries. This was introduced in #570.
I'm suggesting removing the retry behavior because:
-
It doesn't work as implemented. I was going to try to refactor this to make the code more readable, so I started writing tests. The retries don't actually happen as promised. First, the 'full' option for on_retry doesn't actually start again from the initial condition, and the other options wouldn't behave as desired, either: initial is trajectory, not a copy of it. Second, if the number of retries is supposed to be number of REtries (that is, 2 retries means 3 total trials), it's off by 1. Third, the max length retries absolutely never happen under any circumstances -- there's no way for them to trigger a new trajectory for the retrial.
-
Whether to retry seems to me like higher-level behavior that could be controlled by a user outside our engine. We raise the errors, the user handles them (including retries, if desired). OPS, as a "user" of the engines, does handle these errors, and retries are incorrect in path sampling. So an end-user who changes the engine parameters could get bad path sampling.
In other words, this doesn't break backward compatibility because the feature didn't work anyway and it prevents users from accidentally doing something that will break detailed balance. The big idea is that instead of handling errors internally, the inner loops will throw an error that can be caught. For cases where retrying is appropriate, the "user" code can handle the retry.
@jhprinz: Thoughts? Can I remove the retry behavior when I refactor the error management?
The
engine.iter_generatemethod has a bunch of error handling, including things to manage retries. This was introduced in #570.I'm suggesting removing the retry behavior because:
It doesn't work as implemented. I was going to try to refactor this to make the code more readable, so I started writing tests. The retries don't actually happen as promised. First, the
'full'option foron_retrydoesn't actually start again from the initial condition, and the other options wouldn't behave as desired, either:initialistrajectory, not a copy of it. Second, if the number of retries is supposed to be number of REtries (that is, 2 retries means 3 total trials), it's off by 1. Third, the max length retries absolutely never happen under any circumstances -- there's no way for them to trigger a new trajectory for the retrial.Whether to retry seems to me like higher-level behavior that could be controlled by a user outside our engine. We raise the errors, the user handles them (including retries, if desired). OPS, as a "user" of the engines, does handle these errors, and retries are incorrect in path sampling. So an end-user who changes the engine parameters could get bad path sampling.
In other words, this doesn't break backward compatibility because the feature didn't work anyway and it prevents users from accidentally doing something that will break detailed balance. The big idea is that instead of handling errors internally, the inner loops will throw an error that can be caught. For cases where retrying is appropriate, the "user" code can handle the retry.
@jhprinz: Thoughts? Can I remove the retry behavior when I refactor the error management?