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 issues #258 and #259 #260

Merged
merged 13 commits into from Mar 20, 2024
Merged

Fix issues #258 and #259 #260

merged 13 commits into from Mar 20, 2024

Conversation

lohedges
Copy link
Contributor

@lohedges lohedges commented Mar 8, 2024

This PR...

  1. closes [BUG] Ions should be excluded in AMBER position restraint mask #258 by excluding the standard free ions (Na and Cl) to the AMBER position restraint mask. This logic is triggered when the manual restraint mask exceeds the mask character limit, so we need to fall back on the assumption of standard AMBER atom naming. This will work in the case of a BioSImSpace workflow, e.g. where solvation is performed with BioSimSpace, but not for arbitrary input. (The restraint is unreliable then anyway.)

  2. closes [BUG] The __pow__ operator for BioSimSpace.Types._GeneralUnit doesn't handle fractional exponents #259 by adding support for fractional exponents in the BioSimSpace.Types._GeneralUnit.__pow__ operator. I've also re-orded the dimension mask used internally for consistency with sire, which will make it easier to create GeneralUnit types from these masks. (I hadn't used this before.) These update is largely covered by the existing tests but I've added a further test specific to fractional exponents to cover a range of cases, including error handling.

  • 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 bug Something isn't working exscientia Related to work with Exscientia labels Mar 8, 2024
@lohedges lohedges requested a review from chryswoods March 8, 2024 20:54
@lohedges
Copy link
Contributor Author

lohedges commented Mar 9, 2024

Looks like I just need to update a unit mask in the ABFE restraint code in the Sandpit. Will fix on Monday.

chryswoods
chryswoods previously approved these changes Mar 19, 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.

Looks great - I like how pow has let you remove special case code elsewhere. Nice use of rounding. I need to remember to port this down into sire too.

Good unit tests :-)

@lohedges
Copy link
Contributor Author

Thanks. Yes, this simplified a lot of stuff elsewhere so it was worth doing, regardless of whether it will be needed in the long term.

@lohedges lohedges dismissed chryswoods’s stale review March 20, 2024 08:39

The merge-base changed after approval.

@lohedges lohedges merged commit 4480a5a into devel Mar 20, 2024
5 checks passed
@lohedges lohedges deleted the fix_258_259 branch March 20, 2024 08:39
lohedges added a commit that referenced this pull request Mar 20, 2024
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
2 participants