Remember first step of moving restraint #486
Conversation
|
A VMD test fails with a segfault. Locally I can't replicate the segfault but I do get an error:
which is due to a negative lambda, because VMD does not guarantee that timestep numbers increase over time. Not sure how to handle this: generally speaking, time-dependent biases don't behave correctly in VMD in the overwhelming majority of cases. In the case of moving restraints, we have no information on the mapping between VMD frames and simulation timesteps, so there is no way to recover the right time dependence. Should time-dependent biases just freeze when |
|
I didn't look closely at that job, but in many cases it is spiff that segfaults when the differences are too big... :-/ I'm okay with turning those biases into static ones for VMD purposes. |
jhenin
force-pushed
the
moving_restr_first_step
branch
from
dcb0b5c
to
d8917fe
Compare
jhenin
force-pushed
the
moving_restr_first_step
branch
2 times, most recently
from
4db2349
to
cbeaf92
Compare
|
Freshly rebased onto master. Provided that the tests pass, I'll request a final review @giacomofiorin |
jhenin
force-pushed
the
moving_restr_first_step
branch
from
cbeaf92
to
4f9590d
Compare
|
Well, after fixing my oversight, we have passing tests! Ready for merging IMO. |
There was a problem hiding this comment.
The overall change is justified and clearly implemented.
About the part that affects VMD specifically (disabling changes in restraint centers or force constant), I think it should be done consistently across classes and documented. See specific comment.
One "nice to have" feature may be optionally changing the step counter via scripting commands in VMD, i.e. to "simulate a simulation". But that's clearly outside the scope of this PR.
src/colvarbias_restraint.cpp
Outdated
| if (cvm::main()->proxy->simulation_running()) { | ||
| // update parameters (centers or force constant) | ||
| error_code |= colvarbias_restraint_centers_moving::update(); | ||
| error_code |= colvarbias_restraint_k_moving::update(); | ||
| } |
There was a problem hiding this comment.
@jhenin For consistency I think this check should also be done for the other derived restraint classes, i.e. colvarbias_linear and colvarbias_harmonic_walls.
If so, maybe a simpler approach would be to do the check inside the two *_moving::update() methods in the base classes, so that derived restraint classes automatically get it. But I'm not hung up on this latter suggestion if there's a strong argument against.
jhenin
force-pushed
the
moving_restr_first_step
branch
from
4f9590d
to
6684504
Compare
|
@giacomofiorin followed your advice and moved the test into the update_* functions |
This update consists exclusively of bugfixes or maintenance-related changes. The following is a list of pull requests in the Colvars repository since the previous update to LAMMPS: - 532 Add XYZ trajectory reading feature Colvars/colvars#532 (@jhenin, @giacomofiorin) - 531 Delete objects quietly, unless explicitly requested via script (including VMD) Colvars/colvars#531 (@giacomofiorin) - 530 Append newline to log and error messages if not already present Colvars/colvars#530 (@giacomofiorin) - 528 Forward-declare OpenMP lock Colvars/colvars#528 (@giacomofiorin) - 527 Remove unneeded STL container Colvars/colvars#527 (@giacomofiorin) - 526 Allow collecting configuration files and strings before setting up interface Colvars/colvars#526 (@giacomofiorin, @jhenin) - 523 Fallback to linearCombination when customFunction is missing in customColvar Colvars/colvars#523 (@HanatoK, @giacomofiorin) - 522 Use iostream::fail() to check for I/O error Colvars/colvars#522 (@jhenin) - 520 Fix ref count Colvars/colvars#520 (@giacomofiorin) - 513 Set target temperature through a common code path Colvars/colvars#513 (@giacomofiorin, @jhenin) - 509 Safer detection of Windows with recent Microsoft Visual Studio versions Colvars/colvars#509 (@akohlmey) - 508 Update LAMMPS patching method to reflect Lepton availability Colvars/colvars#508 (@giacomofiorin) - 497 Increase the precision of write_multicol Colvars/colvars#497 (@HanatoK) - 496 Only perform MTS automatic enable/disable for timeStepFactor > 1 Colvars/colvars#496 (@giacomofiorin) - 493 Remove unused branch of quaternion input function Colvars/colvars#493 (@giacomofiorin) - 489 Ensure there are spaces between the fields in the header Colvars/colvars#489 (@HanatoK) - 487 Use map of output streams, and return references to its elements Colvars/colvars#487 (@giacomofiorin, @jhenin) - 486 Remember first step of moving restraint Colvars/colvars#486 (@jhenin) - 485 Add decoupling option for moving restraints Colvars/colvars#485 (@jhenin) - 483 Update Lepton via patching procedure Colvars/colvars#483 (@giacomofiorin) - 481 Make file-reading operations of input data abstractable Colvars/colvars#481 (@giacomofiorin) Authors: @akohlmey, @giacomofiorin, @HanatoK, @jhenin
This is needed for correct handling of moving restraints (continuous, using targetNumSteps) that don't begin at step 0.