Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cv save changes prefix for future output #683

Open
jhenin opened this issue Apr 9, 2024 · 8 comments
Open

cv save changes prefix for future output #683

jhenin opened this issue Apr 9, 2024 · 8 comments
Assignees

Comments

@jhenin
Copy link
Member

jhenin commented Apr 9, 2024

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

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

jhenin commented Apr 9, 2024

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

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

jhenin commented Apr 12, 2024

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

  • cv save <prefix> saves only the global state file
  • cv bias <name>save <prefix> (already exists)
  • cv bias <name> writeoutput <prefix> to write the output files of the given bias only

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

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

jhenin commented Apr 12, 2024

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

@jhenin
Copy link
Member Author

jhenin commented Apr 12, 2024

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

giacomofiorin commented Apr 12, 2024

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.

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

No branches or pull requests

2 participants