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

Allow using a Colvars input state file instead of binary checkpoint in GROMACS #610

Merged
merged 10 commits into from
Nov 15, 2023

Conversation

giacomofiorin
Copy link
Member

@giacomofiorin giacomofiorin commented Oct 21, 2023

See discussion at: https://gitlab.com/gromacs/gromacs/-/merge_requests/3671#note_1577988430

This PR implements that requirement, but in a different manner than was mentioned before: a new keyword internal to the Colvars config file that specifies a state file to be used unless a state is read in any other way, as provided by the MD engine. After the a state is read the first time, the default input state file name is deleted (so that it won't be loaded again).

The new keyword is currently not tested in CI yet. I feel that it would be confusing to expose it to engines other than GROMACS, which don't need it. Currently, it's only documented for that engine. @jhenin What are your thoughts on TinkerHP?

Note below how the two engines that provide their own restart file differ significantly:

  • GROMACS needs a way to continue a simulation when a Colvars config file is changed or when a different build of GROMACS is used (both of which prevent restarting Colvars from the checkpoint file). The proposed change would be to use a default state file for Colvarskeyword colvars-inputstatefile in the MDP, which will be superseded when a checkpoint file is read.
  • LAMMPS currently allows (since Unformatted (binary) checkpointing #558) restarting from a Colvars input state file or the LAMMPS restart file. When both are provided, the former overrides the latter, i.e. this behavior is opposite to what seems most appropriate for GROMACS.

While I finish the implementation of this feature for GROMACS, I'd appreciate feedback on whether the above inconsistency should be resolved or not. Mainstream LAMMPS does not contain yet the Colvars code currently in master, so there is time to change.
EDIT: see here #610 (comment)

@giacomofiorin
Copy link
Member Author

Note: converting to draft because the first commit undoes the TPR -> EDR change, this is a conflict that needs to be resolved.

@jhenin
Copy link
Member

jhenin commented Oct 31, 2023

The issue is, LAMMPS lets the user specify new options for each run even after a restart, so if they mention a state file, it should have priority status - whereas Gromacs doesn't, so if a cpt file can be read with success, the state file could be obsolete from a previous run.

@giacomofiorin giacomofiorin force-pushed the input-state-format-priority branch 4 times, most recently from efec113 to ccfae0d Compare November 8, 2023 20:58
@giacomofiorin giacomofiorin changed the title Allow using a Colvars input state file instead of binary checkpoint in a consistent manner Allow using a Colvars input state file instead of binary checkpoint in GROMACS Nov 8, 2023
@giacomofiorin giacomofiorin marked this pull request as ready for review November 8, 2023 22:28
@giacomofiorin
Copy link
Member Author

Merge conflicts resolved, so I was able to add a needed change, i.e. preventing the GROMACS proxy from setting the initial step of the simulation when it doesn't know it anyway. I think that should be addressed separately, but out of scope for this PR.

Also removed the LAMMPS tag since we agreed that the apparent inconsistency is not confusing.

Copy link
Member

@jhenin jhenin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Just two suggestions on the documentation.

doc/colvars-refman-main.tex Outdated Show resolved Hide resolved
doc/colvars-refman-main.tex Outdated Show resolved Hide resolved
@giacomofiorin
Copy link
Member Author

@jhenin I made the doc edits and restricted the new keyword only to the GROMACS >= 2024 case.

I leave open in case you have suggestions about TinkerHP, otherwise I'm good with merging.

@jhenin
Copy link
Member

jhenin commented Nov 15, 2023

Colvars/Tinker-HP reads a colvars.state file based on its prefix. I don't think there are specific needs on that side.

@jhenin jhenin merged commit 35bd1dc into master Nov 15, 2023
33 checks passed
@jhenin jhenin deleted the input-state-format-priority branch November 15, 2023 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants