Colvars / colvars

Collective variables library for molecular simulation and analysis programs
http://colvars.github.io/
GNU Lesser General Public License v3.0
196 stars 56 forks source link

cv save changes prefix for future output #683

Open jhenin opened 3 months ago

jhenin commented 3 months ago

From the name and the documentation, I expect "cv save" to save files to the given prefix when called, with no further side effects. In practice, it changes the file names of all future output.

The documentation could be updated, but I'd rather change the behavior to what is expected, maybe providing a separate method of changing the output prefix persistently.

giacomofiorin commented 3 months ago

That sounds good to me. However, without extensive refactoring that in practice means that biases will need to set output_prefix to the argument of cv save, then set it back to the "persistent" value.

Currently, how biases set output_prefix is highly non-uniform: ABF does this in update(), metadynamics in setup_output() and all other biases in the base class init() function. Before making any changes, these should be standardized, otherwise the likelihood of new bugs would be significant...

jhenin commented 3 months ago

If cv save only writes a state file, do biases have to be made aware of the new, temporary prefix in this case?

I agree with the idea of making that behavior more uniform anyway, ideally by migrating it to the base class.

giacomofiorin commented 2 months ago

If cv save only writes a state file, do biases have to be made aware of the new, temporary prefix in this case?

I believe not, save for possible edge cases. However, the documented behavior of cv save is to also save other output files, which do need a prefix. So what you are describing should be a separate command?

I agree with the idea of making that behavior more uniform anyway, ideally by migrating it to the base class.

Sounds good!

jhenin commented 2 months ago

I can't imagine a real usage case for this. The best interface IMO would be:

These can be combined easily to get the current functionality, but they are more fine-grained and easier to document. I don't see a use case for persistently changing the output prefix, but that could be a separate command if there is a specific need.

giacomofiorin commented 2 months ago

I don't see a use case for persistently changing the output prefix, but that could be a separate command if there is a specific need.

NAMD changes the output prefix permanently through the outputName Tcl command...

jhenin commented 2 months ago

Then I'll add a command to change it as well.

jhenin commented 2 months ago

cvm_output_prefix in colvarmodule looks like a remnant of a previous design, and conflicts with output_prefix_str in colvarproxy_io. Ok to remove it?

giacomofiorin commented 2 months ago

cvm_output_prefix in colvarmodule looks like a remnant of a previous design, and conflicts with output_prefix_str in colvarproxy_io. Ok to remove it?

Yes, that's definitely the case!

At this point we can just support either of these two options: (a) the output prefix defined by the engine, or (b) a one-time prefix specified as argument of the scripting functions. You'll probably need to add an argument to colvarbias::write_output_files().

For option (a) we could even consider calling a virtual function from inside colvarproxy_io::output_prefix(), so that whenever the Colvars library code tries to get the output prefix of the engine, it will be up to date? That may remove the need to add a scripting command just to set the default output prefix.