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 #148 #149

Merged
merged 5 commits into from
Sep 19, 2023
Merged

Fix issue #148 #149

merged 5 commits into from
Sep 19, 2023

Conversation

lohedges
Copy link
Contributor

@lohedges lohedges commented Aug 2, 2023

This PR closes #148 and closes michellab/BioSimSpace#427. The code has been updated to correctly update the water topology to match the GROMACS naming convention in both the Gro87 and GroTop files. Since a user may wish to preserve a particular naming convention, e.g. for crystal waters, I've added an option to disable the topology swap if desired, e.g. if they are just writing to file. A few optimisations have been added following the addition of this parameter, since it means that we can avoid double checking the topology in cases where we know that it has already been swapped, e.g. during the setup of a simulation with a particular engine.

I've not resolved the issue of the Gro87 file being parsed incorrectly. I'm not sure what we can do about this (in a general way, at least) since the file is valid (doesn't trip the existing error handling), but is formatted incorrectly.

  • 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 permission to release this code under the GPL3 license: [y]

Suggested reviewers:

@chryswoods

@lohedges lohedges added the bug Something isn't working label Aug 2, 2023
@lohedges lohedges temporarily deployed to biosimspace-build August 2, 2023 11:20 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build August 2, 2023 11:20 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build August 2, 2023 11:20 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build August 2, 2023 11:20 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build August 2, 2023 11:20 — with GitHub Actions Inactive
@lohedges
Copy link
Contributor Author

lohedges commented Aug 3, 2023

It looks like the match_water option won't work correctly for the case of writing to GroTop format when different naming schemes are used for the same water topology within a system, e.g. if you have some crystal waters along with regular waters. The GroTop parser deduplicates the molecule types, so you'll only end up with a single type in the top file.

@lohedges
Copy link
Contributor Author

lohedges commented Aug 7, 2023

This has been consumed into a larger crystal water related feature that I'm working on so I'll close this when I submit that PR. I'll still pull out the specific fix implemented here for a backport, i.e. forgetting to update the water topology for Gro87 format on write.

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.

👍

@lohedges lohedges merged commit e5ae912 into devel Sep 19, 2023
@lohedges lohedges deleted the fix_148 branch October 30, 2023 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants