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 #312 #313

Merged
merged 3 commits into from
Jul 17, 2024
Merged

Fix issue #312 #313

merged 3 commits into from
Jul 17, 2024

Conversation

lohedges
Copy link
Contributor

This PR fixes #312 by skipping RMSD alignment when merging monatomic ions with other molecules, e.g. water when creating an alchemical ion. When a single item mapping is detected and one of the molecules has a single atom, then we skip the alignment and simply replace the coordinates of the mapped atom with those of the reference. A similar approach could be applied in the mapping/merging code with Sire.

(Note that there is some code duplication below since we still have two scoring functions, one for RDKit mappings, and one for Sire.)

  • 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 cresset Related to work with Cresset labels Jul 11, 2024
@lohedges lohedges requested a review from chryswoods July 11, 2024 11:01
@lohedges lohedges had a problem deploying to biosimspace-build July 11, 2024 11:02 — with GitHub Actions Failure
@lohedges lohedges had a problem deploying to biosimspace-build July 11, 2024 11:02 — with GitHub Actions Failure
@lohedges lohedges temporarily deployed to biosimspace-build July 11, 2024 11:02 — with GitHub Actions Inactive
@lohedges lohedges had a problem deploying to biosimspace-build July 11, 2024 11:02 — with GitHub Actions Failure
@lohedges lohedges had a problem deploying to biosimspace-build July 11, 2024 11:02 — with GitHub Actions Failure
@lohedges lohedges had a problem deploying to biosimspace-build July 11, 2024 11:55 — with GitHub Actions Failure
@lohedges lohedges had a problem deploying to biosimspace-build July 11, 2024 11:55 — with GitHub Actions Failure
@lohedges lohedges temporarily deployed to biosimspace-build July 11, 2024 11:55 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build July 11, 2024 11:55 — with GitHub Actions Inactive
@lohedges lohedges had a problem deploying to biosimspace-build July 11, 2024 11:55 — with GitHub Actions Failure
@lohedges
Copy link
Contributor Author

Bah, it looks like all of the AMBER related tests on Linux have inexplicably started failing. It appears that the sander.LES executable is being used, so perhaps this isn't working as expected, or behaves differently for the same input. Will try to debug locally. Amazing that this happens two days after release.

FAILED tests/Process/test_amber.py::test_minimise[backbone] - AssertionError: assert not True
 +  where True = <bound method Process.isError of BioSimSpace.Process.Amber(<BioSimSpace.System: nMolecules=631>, BioSimSpace.Protocol.Minimisation(steps=100, restraint='backbone', force_constant=10.0 kcal_per_mol/angstrom**2), exe='$PREFIX/bin/sander.LES', name='test', work_dir='/tmp/tmp5cpac605', seed=None)>()

@lohedges
Copy link
Contributor Author

Nope, it's the exact same version of ambertools, which matches the version that I have locally:

    ambertools:                       23.6-cuda_None_nompi_py310ha112c55_105 conda-forge

Everything is fine locally, and for the builds the other day. If a single Python variant failed then I'd think that it was a freak occurrence, but all of them did 🤷‍♂️

@lohedges lohedges temporarily deployed to biosimspace-build July 11, 2024 12:05 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build July 11, 2024 12:05 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build July 11, 2024 12:05 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build July 11, 2024 12:05 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build July 11, 2024 12:05 — with GitHub Actions Inactive
@lohedges
Copy link
Contributor Author

Looks like the issue is that sander.LES is the first executable with sander that it's detecting during the CI. I've excluded LES from that match. Fingers crossed it works.

@lohedges
Copy link
Contributor Author

Yes, that did the trick. Really confused why this just started happening. Would have been nice if it happened before the release, since it's possible that people creating a fresh environment will experience the problem.

@lohedges lohedges merged commit 14e376c into devel Jul 17, 2024
5 checks passed
@lohedges lohedges deleted the fix_312 branch July 17, 2024 10:18
lohedges added a commit that referenced this pull request Jul 17, 2024
lohedges added a commit that referenced this pull request Jul 19, 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 cresset Related to work with Cresset
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Unable to merge ions
1 participant