colvars icon indicating copy to clipboard operation
colvars copied to clipboard

[RFC] More band-aid fixes to Colvars

Open HanatoK opened this issue 7 months ago • 12 comments

Well, I believe that no one would really like this PR. This PR uses a lot of hacks and workarounds in the backend to achieve reusable CVCs in SMP with limitations as less as possible (although I am not a big fan of distributing CVCs over threads), including:

  1. Build a toy AST and determine the parallelization scheme by the depth of the node. At first glance, colvardeps looks like an AST but after playing with it I feel it is not a real one, and it really just checks the dependencies of features, and there is no true AST. It would be better if Colvars could be redesigned with a true AST and a dependency checker for it. The dependency checker should not own the AST;
  2. Bypass the colvar class and take the cvc objects out to build the AST. I think that the colvar class should be completely removed;
  3. I don't know why the smp_lock and smp_unlock in colvarproxy_namd are implemented as creating and destroying locks, so I have changed them;
  4. Implement the chain rule in a dirty manner (see colvar::cvc::modify_children_cvcs_atom_gradients and propagate_colvar_force). When calling calc_gradients and apply_force of a CVC consisting of sub-CVCs, it now propagates the gradients and forces to all its sub-CVCs;
  5. To avoid race condition in propagating the atom gradients when reusing CVCs, I have to use smp_lock. However, it is very coarse-grained so I expect an additional performance penalty. I thought there should be a lock tied to each atom group but found none.

In summary, I think that Colvars should be fundamentally changed to achieve better support of reusable components and parallelization.

This PR tries to solve #232, extends #644 and finishes:

  • [x] Reusing the computation of the individual "nodes" in a pair of path CVs ("s" and "z").

HanatoK avatar Jun 29 '24 22:06 HanatoK