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

Feature reduce #175

Merged
merged 13 commits into from
Sep 22, 2023
Merged

Feature reduce #175

merged 13 commits into from
Sep 22, 2023

Conversation

lohedges
Copy link
Contributor

This PR exposes functionality related to the changes to sire.legacy.Vol.TriclinicBox, allowing the user to manually rotate the space and perform a lattice reduction. Keywords have also been added to BioSimSpace.IO.readMolecules to allow the user to perform these operations on load.

  • 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 test for any new functionality in this pull request: [y]
  • I confirm that I have added documentation (e.g. a new tutorial page or detailed guide) for any new functionality in this pull request: [y]
  • I confirm that I have permission to release this code under the GPL3 license: [y]

Suggested reviewers:

@chryswoods

@lohedges lohedges added the enhancement New feature or request label Sep 22, 2023
@lohedges lohedges requested a review from chryswoods September 22, 2023 10:54
@lohedges lohedges added this to the 2023.4.0 milestone Sep 22, 2023
chryswoods
chryswoods previously approved these changes Sep 22, 2023
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.

Looks good. I think it is good that we assume the perturbable properties are called xxx0 and xxx1 (as you've done to find the perturbable molecules coordinates and velocities properties). The user has freedom to use other names, but it makes the code more difficult. I've said this is the format in the new sire docs for merged molecules. It may be worth separately creating a "detailed guide" page on the BioSimSpace website describing the merged molecule layout and format.

@lohedges
Copy link
Contributor Author

Yes, that's a good point. I allow the user to pass a map when performing the merge (if the system has different names) but then it just uses what it finds and appends 0 and 1.

@lohedges
Copy link
Contributor Author

Hmm, apparently the updated test angles weren't on this branch, despite me merging across from devel. Will see what went wrong.

@lohedges
Copy link
Contributor Author

Nope, the change is there and it's built against the most recent development version of Sire. Will debug locally. I can't see that the recent Sire merge has somehow reverted the triclinic update?

@lohedges
Copy link
Contributor Author

(No, not possible since other tests would fail here.)

@lohedges
Copy link
Contributor Author

Ah, the issue is that the test works if you run it in isolation (as I was doing) but not if you run them all. Presumably the space is being modified by another test. Will fix and repush.

@lohedges lohedges temporarily deployed to biosimspace-build September 22, 2023 11:51 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build September 22, 2023 11:51 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build September 22, 2023 11:51 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build September 22, 2023 11:51 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build September 22, 2023 11:51 — with GitHub Actions Inactive
@lohedges lohedges merged commit 3a4c5ca into devel Sep 22, 2023
5 checks passed
@lohedges lohedges deleted the feature_reduce branch September 22, 2023 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants