Fix issue #236

[lohedges]

This PR closes #236 and closes OpenBioSim/biosimspace_tutorials#23. This is achieved by making the _generate_binary_run_file method static, so that it can be called externally, e.g. during the BioSimSpace.FreeEnergy.Relative setup process. This is tested via the existing BioSimSpace tests, as well as the tutorial notebook for which the original issue was posted.

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

Actually, I need to adjust the port to the Exscientia Sandpit since they allow the use of a reference coordinate file for the position restraints. (This isn't covered in the tests.) The failures are the existing random search failure that only occurs on the CI (intermittently) for certain Python variants on Linux.

[lohedges]

Okay, looks like I'll need to remove the black check on Python 3.9 since it's now using functionality that's deprecated.

The Windows error is weird. It seems to be repeatedly failing to read certain files that were fine before. No changes are related to these.

[lohedges]

The Windows failure appears to be a different file each time. I'm wondering if it's a RAM or storage issue during conda build. Trying to see if any of the runner settings have changed.

[lohedges]

Now the Python 3.9 import test is repeatedly failing on Linux. (No error printed unfortunately.) Will need to debug locally to see what the issue is. Possibly some deprecated functionality in one of our dependencies?

[lohedges]

The Windows issue appears to have magically fixed itself. The other failure is due to deprecated functionality in pytest 8 now rasing an exception. I'll need to pin to version 7 until pytest-black is updated. That said, it might be worth thinking about removing this from the build and using something like pre-commit instead.

[lohedges]

Having fixed that, I now see the weird search result error on two builds 🤦‍♂️

FAILED tests/Sandpit/Exscientia/_SireWrappers/test_system.py::test_restraint_atoms[backbone-expected0] - assert [6, 8, 14, 15, 16] == [4, 5, 6, 8, 14, 15, 16]
  At index 0 diff: 6 != 4
  Right contains 2 more items, first extra item: 15
  Full diff:
  - [4, 5, 6, 8, 14, 15, 16]
  ?  ------
  + [6, 8, 14, 15, 16]
FAILED tests/_SireWrappers/test_system.py::test_restraint_atoms[backbone-expected0] - assert [6, 8, 14, 15, 16] == [4, 5, 6, 8, 14, 15, 16]
  At index 0 diff: 6 != 4
  Right contains 2 more items, first extra item: 15
  Full diff:
  - [4, 5, 6, 8, 14, 15, 16]
  ?  ------
  + [6, 8, 14, 15, 16]

[lohedges]

And now everything passes 🤷‍♂️

[chryswoods]

Strange - I agree about switching to pre-commit. I moved to ruff locally, and its very small differences to black cause occasional reformats (and "reformat wars" on commonly edited files). A pre-commit would give us a canonical format that would be independent of person and IDE (i.e. independent of my local changes and inconsistencies ;-))

[chryswoods]

Looks good - I tend to use Class.static_function() when calling static functions, rather than self.static_function(), as it makes it more obvious that a static function is being called. On my quick scan I didn't get why we were passing lots of state from self into a self.function().

i.e. I would use Gromacs._generate_binary_run_file(self._config_file...) rather than self._generate_binary_run_file(self._config_file...).

This is just my personal preference. I'm happy if you disagree and want to keep it as is.

[lohedges]

In this use case I create an initial process that is the reference for the others, hence re-use all of its attributes when re-running grompp, e.g. making sure we use the same executable, etc. As the instance exists, I just called the method on it. I agree it would be clearer to just call on the class directly.