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 issue #194 #195

Merged
merged 1 commit into from
Jun 2, 2024
Merged

Fix issue #194 #195

merged 1 commit into from
Jun 2, 2024

Conversation

lohedges
Copy link
Contributor

This PR closes #194 by skipping BioSimSpace position restraint include directives when parsing GROMACS topology files. I'm not sure if there is a more general way of fixing this issue, but this will work for the purposes of being able to read GROMACS files that were written as input to a BioSimSpac.Process.Gromacs simulation using position restraints. Here the restraints will be ignored, but they would be re-written if the user wanted to run the same protocol. In this instance, they just want to be able to read the system back in, i.e. the topology.

  • I confirm that I have merged the latest version of devel into this branch before issuing this pull request (e.g. by running git pull origin devel): [y]
  • I confirm that I have added a changelog entry to the changelog (we will add a link to this PR as part of the review): [y]
  • I confirm that I have permission to release this code under the GPL3 license: [y]

Suggested reviewers:

@chryswoods

@lohedges lohedges added bug Something isn't working exscientia Related to work with Exscientia labels Apr 30, 2024
Copy link
Contributor

@chryswoods chryswoods left a comment

Choose a reason for hiding this comment

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

Are these restraints used only by BioSimSpace? If so, then I am happy to approve. Just want to check that we aren't ignoring something that another program would expect us to read.

@lohedges
Copy link
Contributor Author

This is the standard way of adding position restraints in GROMACS topology files, so similar directives might be present in other files too (which would also cause the parser to fail). Here I've explicitly skipped the records when the exact BioSimSpace naming convention is found, but I could also update that to be more unique, e.g. include bss in the string.

Another solution would be adding support for parsing these records, but I'm not sure how useful that would be.

@chryswoods
Copy link
Contributor

Ok - we can use this skip for now, while I think about how to add support for parsing the record. It may be useful to have these as extra properties of the molecule or system.

@chryswoods chryswoods merged commit 12a8538 into devel Jun 2, 2024
4 of 5 checks passed
@chryswoods chryswoods deleted the fix_194 branch June 2, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working exscientia Related to work with Exscientia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] GroTop parser fails when position restraint include directives are present
2 participants