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

Avoid setting all end-state properties when decoupling #253

Merged
merged 2 commits into from
Feb 27, 2024

Conversation

fjclark
Copy link
Contributor

@fjclark fjclark commented Feb 26, 2024

This closes #252. BSS.Align.decouple() no longer sets LJ1, element1, or ambertype1, rather this is done by the _to_pert_file method of Process.Somd. The related tests of "decouple" have been removed, and direct tests on the SOMD pert files have been added.

  • 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:

@lohedges, @xiki-tempula

This addresses OpenBioSim#252.
decouple() no longer sets LJ1, element1, or ambertype1, rather
this is done by the _to_pert_file method of Process.Somd. The related
tests of "decouple" have been removed, and direct tests on the SOMD
pert files have been added.
Copy link
Contributor

@xiki-tempula xiki-tempula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lohedges
Copy link
Contributor

Looks good. The test failure on Python 3.11 was because of pytest-black. It appears to be unhappy with the LaTeX formatting in one of your matplotlib commands. Could you test this locally to see if there's a workaround?

@fjclark fjclark temporarily deployed to biosimspace-build February 27, 2024 10:07 — with GitHub Actions Inactive
@fjclark fjclark temporarily deployed to biosimspace-build February 27, 2024 10:07 — with GitHub Actions Inactive
@fjclark fjclark temporarily deployed to biosimspace-build February 27, 2024 10:07 — with GitHub Actions Inactive
@fjclark fjclark temporarily deployed to biosimspace-build February 27, 2024 10:07 — with GitHub Actions Inactive
@fjclark fjclark temporarily deployed to biosimspace-build February 27, 2024 10:07 — with GitHub Actions Inactive
@fjclark
Copy link
Contributor Author

fjclark commented Feb 27, 2024

Thanks for the comment @lohedges. My understanding was that the error was due to formatting in _somd.py, and that the matplotlib issues were just warnings (from files that I haven't changed). Is there an error message I've missed for the matplotlib commands?

I'm confused, because the error message I could see seems to show that black is unhappy with an extra line:

 --- $SRC_DIR/python/BioSimSpace/Sandpit/Exscientia/Process/_somd.py	2024-02-26 19:31:10.766528+00:00
+++ $SRC_DIR/python/BioSimSpace/Sandpit/Exscientia/Process/_somd.py	2024-02-26 19:36:43.213044+00:00
@@ -1032,11 +1032,10 @@
     mol = molecule._sire_object.__deepcopy__()
 
     # If the molecule is decoupled (for an ABFE calculation), then we need to
     # set the end-state properties of the molecule.
     if molecule.isDecoupled():
-
         # Invert the user property mappings.
         inv_property_map = {v: k for k, v in property_map.items()}
 
         # Get required properties.
         lj = inv_property_map.get("LJ", "LJ")

but I auto-format with black on saving, and running black directly on the file left it unchanged. Could a different version of black be being pulled in here?

To get round this, I've just removed the line.

@lohedges
Copy link
Contributor

Apologies, I misread the error message. You are correct, the matplotlib stuff is just a deprecation warning. The tests are currently run using black 23. Perhaps you have a different version locally. Deleting the is the correct fix.

@lohedges lohedges merged commit 848e90c into OpenBioSim:devel Feb 27, 2024
5 checks passed
@xiki-tempula
Copy link
Contributor

@lohedges Do you mind port this to the main as well, please? Thank you.

@lohedges
Copy link
Contributor

Yes, of course. I was planning on doing that today but didn't get time. Will do it first thing tomorrow.

@xiki-tempula
Copy link
Contributor

Thanks

lohedges added a commit that referenced this pull request Feb 29, 2024
lohedges added a commit that referenced this pull request Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a flag to BSS.Align.decouple to only change the sire property when using this for SOMD
3 participants