Skip to content

Conversation

@jhenin
Copy link
Member

@jhenin jhenin commented Nov 18, 2025

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
Copy link
Member Author

jhenin commented Nov 20, 2025

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.

@giacomofiorin
Copy link
Member

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
Copy link
Member

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.

@HanatoK
Copy link
Member

HanatoK commented Nov 20, 2025

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?

@giacomofiorin
Copy link
Member

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.

@HanatoK
Copy link
Member

HanatoK commented Nov 20, 2025

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.

@jhenin
Copy link
Member Author

jhenin commented Nov 21, 2025

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
Copy link
Member Author

jhenin commented Nov 24, 2025

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
Copy link
Member Author

jhenin commented Dec 5, 2025

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
Copy link
Member Author

jhenin commented Dec 5, 2025

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 jhenin force-pushed the remove_static_pointers branch from e7aca8f to cf9ec7c Compare December 5, 2025 10:32
@jhenin jhenin force-pushed the remove_static_pointers branch from cf9ec7c to ad7df73 Compare December 5, 2025 10:38
@jhenin jhenin force-pushed the remove_static_pointers branch from c04c86b to bcd4afb Compare December 5, 2025 19:06
@jhenin jhenin force-pushed the remove_static_pointers branch from f5fa4a5 to efa5526 Compare December 5, 2025 20:52
about diamond inheritance
@jhenin jhenin force-pushed the remove_static_pointers branch from e6f7cfe to 46122d2 Compare December 5, 2025 21:52
@jhenin
Copy link
Member Author

jhenin commented Dec 7, 2025

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
Copy link
Member Author

jhenin commented Dec 7, 2025

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

@jhenin jhenin marked this pull request as ready for review December 7, 2025 22:47
@jhenin jhenin requested a review from giacomofiorin December 7, 2025 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants