bugfix-ncycles-computed#80
Conversation
Merge pull request michellab#78 from Roy-Haolin-Du/main
Added nmoves as configurable and changed ncycles to property, added t…
|
Fixes #79 - Adds ncycles support to prevent memory errors. |
| ncycles_int = round(float(ncycles)) | ||
|
|
||
| if ncycles_int < 1: | ||
| raise ValueError( |
There was a problem hiding this comment.
I know this was done before for timestep and runtime, but I think it might be better to validate all of this stuff in a validator, rather than in the property. Then, an error will be raised on instantiation/ assignment (we've set it to validate on assignment) rather than later when the user tries to call the property. The property can then just compute ncycles.
| """ | ||
| Make sure runtime is a multiple of timestep | ||
| Calculate number of cycles from runtime, nmoves and timestep. | ||
| Formula: runtime = nmoves × ncycles × timestep |
There was a problem hiding this comment.
I might not be remembering how these settings work, but my main comment is: do we know if this will work with adaptive computations where the assigned times may be pretty short? 25000 * 4 fs = 100 ps which is a fairly large smallest runtime. Have you tried this with the adaptive runs?
A simple way to make this more flexible would be to replace nmoves with max_nmoves. This would modify the behaviour of the previous nmoves property so that the behaviour stays as it was unless runtime > max_nmoves * timestep, in which case the nmoves gets e.g. halved and ncycles doubled or something similar. We'll need to be careful to make sure you get a valid nmoves and ncycles in this case (easiest way might be to enforce that runtime must be a multiple of 2 * timestep).
|
Thanks Roy! Mainly looks good but have left a couple of comments. |
This prevents memory overflow from single cycle long runtimes while keeping
Ready for a version bump. |
|
Hi Finlay @fjclark would you mind taking a quick look? |
| # Works if the calculation has already been setup | ||
| calc.update_engine_config_option(timestep=2.0) # fs | ||
| # Before setup(): modify the engine_config directly | ||
| calc.engine_config.timestep = 2.0 |
There was a problem hiding this comment.
Super minor comment, but probably keep the # fs comments here and below
|
Amazing thanks Roy, looks good to me! Tests look good and thorough! Feel free to merge and bump the version. |
Add # fs comment to timestep examples in guides
|
Thanks, Finlay I’ll merge and bump the version. |
|
close #79 |
Description
Added nmoves as a configurable field and changed ncycles to a computed property
Todos
Added nmoves as a configurable field and changed ncycles to a computed property in
engine_config.pyto prevent memory overflow from single-cycle runtimes.Status