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

fix: use correct distances.dat when starting from existing traj #485

Merged
merged 11 commits into from
Sep 13, 2024

Conversation

jmbuhr
Copy link
Collaborator

@jmbuhr jmbuhr commented Sep 11, 2024

and fix a warning that warned in exactly the wrong case.

@jmbuhr jmbuhr changed the title use correct distances.dat when starting from existing traj fix: use correct distances.dat when starting from existing traj Sep 11, 2024
@github-actions github-actions bot marked this pull request as draft September 11, 2024 13:50
@jmbuhr jmbuhr marked this pull request as ready for review September 11, 2024 13:50
@KRiedmiller
Copy link
Collaborator

This was already working, it's implemented in get_existing_files. What broke it was the moving of the plumed input file in #484. Why was that necessary again? The plumed output can be quite big, so I would not copy it just for tidiness, only if there is a risk of us modifying it (which would be probably wrong anyway?).

@jmbuhr
Copy link
Collaborator Author

jmbuhr commented Sep 13, 2024

No, we that's what we talked about. When you run kimmdy over an existing trajectory it needs to interpret the filepath for distances.dat that it get's from a plumed.dat file relative to said trajectory. This is what this change does and it only does so when required (i.e. there is an xtc/trr/tpr input). get_existing_files interprets the filename from the plumed input relative to the plumed input file, which would be wrong for those cases.

@jmbuhr
Copy link
Collaborator Author

jmbuhr commented Sep 13, 2024

No plumed output is moved, only the input file.

@jmbuhr
Copy link
Collaborator Author

jmbuhr commented Sep 13, 2024

One could argue that the other change, copying the plumed input file, is not necessary, though it increases reproducibility, but this one definitely is.

@KRiedmiller
Copy link
Collaborator

atm KIMMDY does look for the plumed output relative to the plumed input, not relative to old trajectories.
The latest change moved the input, without moving the output; your change now reroutes the path to the correct output. I don't see the need of moving the input and thereby separating the in- and output.
New output from new runs will be relative to new trajectories, so there should be no risk of overwriting something.

@jmbuhr
Copy link
Collaborator Author

jmbuhr commented Sep 13, 2024

atm KIMMDY does look for the plumed output relative to the plumed input, not relative to old trajectories.

Exactly. And that's wrong, because that's not where gromacs put the output. Say I'm analyzing an existing trajectory with the following kimmdy.yml:

gro: 'old_run/eq.gro'
xtc: 'old_run/md.xtc' 
trr: 'old_run/md.trr'
tpr: 'old_run/md.tpr'
plumed: 'assets/plumed.dat'

@jmbuhr
Copy link
Collaborator Author

jmbuhr commented Sep 13, 2024

What behavior would you expect from the above case?

@KRiedmiller
Copy link
Collaborator

KIMMDY looks for this output in different places if starting up and after a md run. When starting it looks (correctly) next to the plumed input file. After an MD, it looks for the output in the run directory of this MD (also correct).

In your example, the distances.dat should be next to the plumed.dat, or on the main branch in the 0_setup dir

@jmbuhr
Copy link
Collaborator Author

jmbuhr commented Sep 13, 2024

distances.dat should be next to the plumed.dat

That's not where gromacs (and gromacs run via kimmdy) puts it. It's relative to gromac's working directory. This is turn can reasonably be assumed to the the parent directory of the trajectory.

@jmbuhr
Copy link
Collaborator Author

jmbuhr commented Sep 13, 2024

In other words: A plumed.dat input file (and it's location) are not sufficient by itself to assume the location of the output file. The only valid location for an output file is in conjunction with the location of the trajectory that was generated using the input file.

@jmbuhr
Copy link
Collaborator Author

jmbuhr commented Sep 13, 2024

cd some/working/dir
gmx mdrun <...> -plumed /some/path/to/plumed.dat

with

... plumed.dat
FILE=distances.dat

produces

some/working/dir/distances.dat

@jmbuhr
Copy link
Collaborator Author

jmbuhr commented Sep 13, 2024

So just like

After an MD, it looks for the output in the run directory of this MD (also correct).

KIMMDY should apply the same logic when starting from an existing trajectory. And while the parent directory of the trajectory is only a heuristic (one could have ran gromacs and specified paths with subdirectories for outputs) it certainly is a better heuristic than looking next to the plumed input file, because those can be re-used and referenced from many simulations.

@jmbuhr jmbuhr requested review from KRiedmiller and removed request for KRiedmiller September 13, 2024 12:38
@jmbuhr jmbuhr merged commit d0c9859 into main Sep 13, 2024
1 check passed
@jmbuhr jmbuhr deleted the plumed-fixes branch September 13, 2024 13:50
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.

2 participants