colvars icon indicating copy to clipboard operation
colvars copied to clipboard

Replace static colvarmodule with class member

Open jhenin opened this issue 1 month ago • 12 comments

In a first stage, just trying to keep functionality intact, removing all static member data from class colvarmodule.

First approach: make a colvarmodule pointer a member of colvarparse, because the big classes inherit from it.

Now a number of lightweight classes (in colvartypes) or sub-classes don't have access to the pointer yet. Alternatives are to extend them with a pointer, or remove the functionality that needs it (mostly log and error calls).

EDIT: because this is so far-reaching, I am turning it into a partial implementation that can be merged in finite time, leaving some of the classes to be modified later.

Namely:

  • colvargrids_ - make broad use of cvm functions, passing pointers to the module between them is very heavy
  • colvarproxy_* base classes. They all implement partial functionality for colvarproxy. I am not sure anymore that inheritance is the best paradigm, or we would need a common base class colvarproxy_base at the root of a diamond inheritance.

colvarbias classes have been handles thanks to virtual inheritance: only the colvarbias base class constructor sets the cvmodule pointer, as passed on by the final classes. Intermediate classes don't have to worry about it.

Final status: the remaining uses of the static pointer can be tracked by looking for calls to cvm::main(). There are new functions log_static and error_static that make use of the static pointer only if available, and have fallbacks if not. All of these can be phased out in later PRs.

jhenin avatar Nov 18 '25 16:11 jhenin

A lot of progress towards the goal. Some classes got access to a colvarmodule pointer. Others got access to a static version of the error that is fatal - mostly assertions detecting bug conditions.

The main hurdle that remains is the scripts, which rely heavily on the static colvarscript_obj() to access the C++ object from C functions. In practice, one script interpreter should interact with only one colvarmodule object through one colvarscript object. I'm not sure how to expose the correct pointer to each interpreter if there are two.

As a first step, we could keep a static, global colvarscript pointer. Admittedly that pushes a part of the problem further down the road, but at least it would let us design everything to work well without static colvamodule pointer from now on.

jhenin avatar Nov 20 '25 21:11 jhenin

This is great progress already! :-)

For Tcl, we could maybe use Tcl_SetAssocData() and Tcl_GetAssocData()? VMD uses this to get a pointer to the main object from the interpreter.

That would only work for Tcl, but that's also where you would need it. LAMMPS scripting commands are associated to a Fix object, so no gymnastics needed

giacomofiorin avatar Nov 20 '25 22:11 giacomofiorin

My main concern is, all Colvars functionality relies on there being a single instance of the module. Removing static pointers would also require adding a different kind of check that there is only one colvarmodule object.

giacomofiorin avatar Nov 20 '25 22:11 giacomofiorin

My main concern is, all Colvars functionality relies on there being a single instance of the module. Removing static pointers would also require adding a different kind of check that there is only one colvarmodule object.

Why does all functionality rely on being a single instance?

HanatoK avatar Nov 20 '25 22:11 HanatoK

Why does all functionality rely on being a single instance?

Mostly for I/O: if multiple instances were allowed, each would need its own set of input and output files.

giacomofiorin avatar Nov 20 '25 22:11 giacomofiorin

Mostly for I/O: if multiple instances were allowed, each would need its own set of input and output files.

But this is what the derived classes of colvarproxy should take care.

HanatoK avatar Nov 20 '25 22:11 HanatoK

In practice, the main application I've had in mind so far would be associating one colvarmodule instance with one molecule in VMD, although that would need more work because 1) VMD has a single Tcl interpreter, and 2) so far we have single proxy object - so we'd have to choose between duplicating the proxy object as well, or let a single proxy switch between molecules. Given the current architecture, I think one proxy per molecule would be simpler.

Thanks to Giacomo's suggestion with Tcl_SetAssocData, I think this is workable: the interpreter could register not just one pointer, but say, one proxy pointer for each loaded molecule. If such a pointer were missing, the script interface would know to allocate a new instance for that molecule.

You may have more ideas for using multiple instances: I'm interested.

jhenin avatar Nov 21 '25 20:11 jhenin

To answer @giacomofiorin 's remark about relying on there being a single instance: my goal here is that several instances could exist and that all calls are addressed to the right objects. One point to be worked out is whether they attach to the same or different proxy objects. For now, I am working under the assumption of a proxy object talking to a single colvarmodule instance.

jhenin avatar Nov 24 '25 08:11 jhenin

Making these changes, I find that the scripting functionality would be better located in the colvarmodule class than colvarproxy. The colvarscript object is now created and used only by the module, and the proxy is just an extra level of indirection.

jhenin avatar Dec 05 '25 09:12 jhenin

I had to re-introduce a patch to ScriptTcl.C to pass the correct colvarscript pointer in the NAMD-defined version of the cv command. Incompatible (non-patched) NAMD will pass a wrong pointer, so I added a validation function to colvarscript to detect that and print a helpful error message.

jhenin avatar Dec 05 '25 10:12 jhenin

I think the last issue in Gromacs is fixed. Note that at present, in shared-memory settings, only one thread has a pointer to the colvarmodule object, but in principle, it could be broadcast or made static so all threads can access it if that was useful.

jhenin avatar Dec 07 '25 21:12 jhenin

Since this is very conflict-prone, I propose to rebase only when ready to merge.

jhenin avatar Dec 07 '25 22:12 jhenin