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

Pass match_water parameter to specialised solvation functions #190

Merged
merged 3 commits into from
Oct 24, 2023

Conversation

lohedges
Copy link
Contributor

This PR fixes a bug where the match_water parameter wasn't passed from the generic BioSimSpace.Solvent.solvate function through to the specialised functions for the supported water models. I've generalised the test so it uses both function types so that we catch similarly missed parameters in future, e.g. when anything else is added.

Closes #185.

  • 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 Oct 23, 2023
@lohedges lohedges requested a review from chryswoods October 23, 2023 18:37
@lohedges lohedges temporarily deployed to biosimspace-build October 23, 2023 18:37 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build October 23, 2023 18:37 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build October 23, 2023 18:37 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build October 23, 2023 18:37 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build October 23, 2023 18:37 — with GitHub Actions Inactive
@lohedges lohedges merged commit 0095314 into devel Oct 24, 2023
5 checks passed
@lohedges lohedges deleted the fix_185_devel branch October 24, 2023 13:00
lohedges added a commit that referenced this pull request Oct 24, 2023
lohedges added a commit that referenced this pull request Oct 24, 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 - Am I right in remembering that the root cause of the bug was the change in the number of positional arguments, so match_water wasn't passed correctly? Is it worth switching to keyword arguments for these many-argument functions?

(no need to do this for this PR fix - but something to consider for the future)

[ignore - posted this because I misread a GH notification - I thought this PR was familiar!]

@lohedges
Copy link
Contributor Author

Yes, that's right. They should really be keywords. They are everywhere other than the validation function, so I'll update that in future. There used to only be a few, but it makes sense to switch to keywords now they have proliferated.

No worries. I forgot to remove the review request since it was identical to the previous PR.

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] match_water parameter not passed through to solvation function
2 participants