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 #124 #125

Merged
merged 3 commits into from
Jul 3, 2023
Merged

Fix issue #124 #125

merged 3 commits into from
Jul 3, 2023

Conversation

lohedges
Copy link
Contributor

@lohedges lohedges commented Jul 3, 2023

This PR closes #124. We now check that the space property of the system is periodic for all supported engines, to ensure that vacuum simulations are run correctly. Note that there is some code duplication here since not all process classes have been ported to the new configuration generator code. The GROMAC code is duplicated since we need to write a modified GRO file for vacuum simulations, which is done in the process setup, not the configuration generator.

  • 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 Jul 3, 2023
@lohedges lohedges requested a review from chryswoods July 3, 2023 11:48
@lohedges lohedges had a problem deploying to biosimspace-build July 3, 2023 11:48 — with GitHub Actions Failure
@lohedges lohedges temporarily deployed to biosimspace-build July 3, 2023 11:48 — with GitHub Actions Inactive
@lohedges lohedges had a problem deploying to biosimspace-build July 3, 2023 11:48 — with GitHub Actions Failure
@lohedges lohedges temporarily deployed to biosimspace-build July 3, 2023 11:48 — with GitHub Actions Inactive
@lohedges lohedges had a problem deploying to biosimspace-build July 3, 2023 11:48 — with GitHub Actions Failure
@lohedges
Copy link
Contributor Author

lohedges commented Jul 3, 2023

Okay, this is failing since there are missing Sire 2023.4.0.dev packages for some platforms and Python variants.

@lohedges
Copy link
Contributor Author

lohedges commented Jul 3, 2023

Looks like I now need to use py39, 310, and 311. However, some packages are still missing, e.g. this doesn't contain a full set.

@lohedges
Copy link
Contributor Author

lohedges commented Jul 3, 2023

I've updated the Python versions but will wait to re-run the CI until the Sire packages are available.

@lohedges lohedges temporarily deployed to biosimspace-build July 3, 2023 13:59 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build July 3, 2023 13:59 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build July 3, 2023 13:59 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build July 3, 2023 13:59 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build July 3, 2023 13:59 — with GitHub Actions Inactive
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.

All good - sorry that this was an annoying change. I think having space and time always defined will help us in the long term

@lohedges
Copy link
Contributor Author

lohedges commented Jul 3, 2023

No problem, it completely makes sense, I just have no idea why this only became an issue post-release, since I was building against a version of Sire where this change was already present 🤷‍♂️

@lohedges lohedges merged commit 5eb871f into devel Jul 3, 2023
@lohedges lohedges deleted the fix_124 branch July 3, 2023 17:22
lohedges added a commit that referenced this pull request Jul 3, 2023
lohedges added a commit that referenced this pull request Jul 3, 2023
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
Development

Successfully merging this pull request may close these issues.

[BUG] Check for non-periodic Cartesian space in process setup
2 participants