Consistent use of PBC functions in NAMD (NAMD-provided vs. internal?)

[giacomofiorin]

Test 022_apathCV_distanceVec fails in the current master. I traced it to the fact that colvarproxy_namd::position_distance() falls back to the base-class method (i.e. the internal PBC functions) because the pointer to the Lattice pointer was uninitialized.

This fall-back was added in commit e767b8d, but it relied implicitly on the internal PBC flag not being properly initialized (thus raising an error). Commit 33da0c9 now initializes such flag, and the error is raised.

I can fix this by letting the NAMD proxy initialize the PBC stuff before initializing the module (commit coming to master). But it would be more robust if we could avoid the if conditional in colvarproxy_namd::position_distance() altogether. @HanatoK any thoughts?

Alternatively, we could avoid using the NAMD-provided PBC functions anyway and always initialize the internal ones. Possibly while also addressing #160 (although to be honest, when dealing with non-orthogonal unit cells I'd much rather use LAMMPS).

EDIT: a straight fix is not possible, again because the GlobalMaster class does not have a valid lattice pointer when initialized. At best, a change in the NAMD repo is required.

[HanatoK]

I second that NAMD should initialize Lattice before Globalmaster. As Lattice is something deduced from the simulation parameters, and Globalmaster or ComputeGlobal works similar to a compute object, initializing the parameters before the compute objects seems logical.
Is it difficult to change the priority of initialization?

[giacomofiorin]

@HanatoK Thinking more about this, I could probably dig up how to pass the Lattice object from SimParameters when the one for the current frame is not up to date.

However, I am not sure that even that would be safe. It is increasingly common to update the simulation frame via scripting commands (i.e. at any time) via commands like reinitatoms. This means that any periodic cell that was set at the time of initialization of the variables may be out of date at the first compute step that follows.

Can we perhaps (1) just check that the pointer is not NULL and throw an error instead of segfaulting, (2) make this error message tell the user to initialize the system in full before defining variables that need the simulation cell, and (3) document this specifically for apathCV?

Since most errors are caught at the first run command, recommending to do run 0 before cv config ... when defining these variable may be simple enough.

[HanatoK]

@giacomofiorin

As for apathCV, Parrinello's paper said "lambda is comparable to the inverse of mean square displacement between succesive frames", so in my implementation it computes the displacements at initialization to provide a default estimation of the lambda parameter. It shoule be noted that their suggestion says "be comparable to" instead of "equals to", and once the lambda parameter is set it cannot be changed during the simulation. Consequently, if the periodic cell changes drastically, the user needs to take care of it and provides a value for lambda instead of relying on the default estimation. If the periodic cell just changes slightly, then there is no need to update lambda again. So I think pass the Lattice object from SimParameters should be enough.

As for other cases that require an updated cell information every step, maybe it is better to pass the periodic cell to Colvars like other atomic information (such as forces and positions), and to use it as a CV.

[giacomofiorin]

@HanatoK Alright, so the PBC functions are only used to compute a default value for lambda from the reference frames, but this value is not needed until the next compute step.

So how about making suggested_lambda in the constructor something non-valid (e.g. a negative number) and then check for this value e.g. in ArithmeticPathBase<...>::computeValue() and initialize it right there if no explicit request was made by the user? At that point you are pretty much guaranteed to get the same PBCs that used for computation.