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

Coordinate interval update #28

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jacknicoludis
Copy link

Description

This PR provides two improvements and one bug fix.

The first improvement is the addition of a coordinates_interval tag to the outputs_config.reporting config, so that the user can specify the output coordinate interval in the xml input file.

The second improvement was allowing the use of the mdtraj HDF5Reporter as a coordinate output file type. This file type is more space efficient than the PDB format, while still containing the topology. See https://mdtraj.org/1.9.4/api/generated/mdtraj.reporters.HDF5Reporter.html#:~:text=HDF5Reporter%20stores%20a%20molecular%20dynamics,be%20stored%20in%20the%20file..

The last change modifies the loading of PDB position using openmm_app.PDBxFile() install of openmm_app.PDBFile() which allows for more reproducible system handling of large systems with more than 99999 atoms.

@jacknicoludis jacknicoludis marked this pull request as ready for review February 13, 2023 20:24
@dyelar
Copy link
Collaborator

dyelar commented Mar 7, 2023

Jack,
Thank you for your contribution. I apologize for the delay. I've looked through the changes. I've made the choice to implement the addition of supporting mmCIF files a bit differently from what you have here, since it also needed to be included in other places within the code, and we already are supporting file type information within the configuration file.

After some discussion with Lane to verify that I was correct, we aren't going to modify the coordinate interval at this time.  The ability to perform a restart cleanly is currently tied to having the coordinate output interval be at the same rate as the gamd logging, and the restart interval.  While not optimal, the addition of this change would break that functionality.

For the H5 output file format, I'm currently evaluating that change to see if we should provide a generalized way of handling it or continue with the one off change.  If we go with just adding hdf5, then we'll have to figure out how to deal with the commit that includes both part of your PDBx input change and an hdf5 output change.

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