Test openfe 1.0.0rc0 + refresh showcase

[mikemhenry]

First spruce up CI, then see if our tests pass using the RC

[github-actions]

👈 Launch a binder notebook on branch OpenFreeEnergy/ExampleNotebooks/openfe-1.0.0rc0-test

[mikemhenry]

url to test on colab https://colab.research.google.com/github/OpenFreeEnergy/ExampleNotebooks/blob/openfe-1.0.0rc0-test/openmm_rbfe/OpenFE_showcase_1_RBFE_of_T4lysozyme.ipynb

NOTE: I've hardcoded the installer into the notebook, so that needs to be changed and point it towards the new env

[mikemhenry]

---------------------------------------------------------------------------
ImportError                               Traceback (most recent call last)
Cell In[23], line 3
      1 # Settings can be accessed from the various classes
----> 3 from openfe.protocols.openmm_rfe.equil_rfe_settings import (
      4     SystemSettings, SolvationSettings, AlchemicalSettings,
      5     OpenMMEngineSettings, AlchemicalSamplerSettings,
      6     IntegratorSettings, SimulationSettings
      7 )
      9 # The documentation on each class can be accessed to know
     10 # what parameters can be set
     11 get_ipython().run_line_magic('pinfo', 'SystemSettings')

ImportError: cannot import name 'SystemSettings' from 'openfe.protocols.openmm_rfe.equil_rfe_settings' ([/home/mmh/micromamba/envs/example-notebook-1.0.0rc0/lib/python3.11/site-packages/openfe/protocols/openmm_rfe/equil_rfe_settings.py](http://localhost:8888/home/mmh/micromamba/envs/example-notebook-1.0.0rc0/lib/python3.11/site-packages/openfe/protocols/openmm_rfe/equil_rfe_settings.py))

First error in the showcase notebook

[hannahbaumann]

I'll start working on fixing that notebook! Should I make a PR into this PR @mikemhenry ?

[richardjgowers]

@hannahbaumann yep I imagine so

[hannahbaumann]

The showcase tutorial goes over how to create settings from the Settings classes directly, instead of through the default options from the protocol. I'm not sure if this is the best way for the user to learn about / access the settings, since we usually say settings should be modified starting from the default option of the protocol. A problem that arises here, e.g. is that some settings (e.g. MultiStateSimulationSettings()) don't have default values for some settings (e.g. equilibration_length), therefore raising an error currently (the default value is defined in the protocol/method, not in the settings). Should I change the showcase to instead teach using and modifying the settings AFTER loading in the default settings from the protocol?

[richardjgowers]

@hannahbaumann Yeah creating defaults then customising these is probably the intended/recommended route. This means we can avoid importing individual sub-settings entirely

[mikemhenry]

@hannahbaumann feel free to either make a PR into this one, or commit directly to the openfe-1.0.0rc0-test branch!

[hannahbaumann]

There are three notebooks in the openmm_rbfe folder, it would be good to check the purpose of each of them (maybe as part of the showcase power hour).

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[mikemhenry]

Hard to debug this one, did it work locally for anyone? I think for now I will just comment it out and make an issue so we can get this merged in

[mikemhenry]

Looks like things work on python 3.10, I updated the notebooks we test as well to setup/ showcase/ cookbook/ openmm_rbfe/ networks/

[mikemhenry]

Hmmm, the openmm_rbfe/OpenFE_showcase_1_RBFE_of_T4lysozyme.ipynb & showcase/openfe_showcase.ipynb notebook times out on osx

[mikemhenry]

For the sake of getting this shipped, I will use some larger result files that could be smaller, but I rather get this out the door then figure that out right now.

[IAlibay]

Small typo

[mikemhenry]

Small typo

Good catch! fixed in 33cca55

[mikemhenry]

Running the colab notebook as a test, if it passes I will merge in the PR... super close now!

[mikemhenry]

 ____________________ cookbook/choose_protocol.ipynb::Cell 6 ____________________
[gw0] linux -- Python 3.10.14 /home/runner/micromamba/envs/openfe-notebooks/bin/python
Notebook cell execution failed
Cell 6: Cell execution caused an exception
Input:
# Double check that the above settings match the defaults - delete this cell if you configure things yourself!
# See https://github.com/OpenFreeEnergy/ExampleNotebooks/issues/138
assert settings == RelativeHybridTopologyProtocol.default_settings()
Traceback:
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
Cell In[1], line 3
      1 # Double check that the above settings match the defaults - delete this cell if you configure things yourself!
      2 # See https://github.com/OpenFreeEnergy/ExampleNotebooks/issues/138
----> 3 assert settings == RelativeHybridTopologyProtocol.default_settings()
AssertionError: 
____________________ cookbook/choose_protocol.ipynb::Cell 7 ____________________
[gw0] linux -- Python 3.10.14 /home/runner/micromamba/envs/openfe-notebooks/bin/python
Notebook cell execution failed
Cell 7: Cell execution caused an exception
Input:
default_settings = RelativeHybridTopologyProtocol.default_settings()
for name, s in settings:
    print(name)
    if getattr(settings, name) != getattr(default_settings, name):
        print(n
Traceback:
  Cell In[1], line 7
    print(n
           ^
SyntaxError: incomplete input

a few time outs and this error is still showing up, but the showcase is working so I will merge it in after one more check on colab!

[mikemhenry]

Fixed the syntax issue and I am updating the cookbook and then we should be good (except for the time outs but we can fix that later)

[mikemhenry]

@richardjgowers I am not sure how to fix this:
time_per_iteration=<Quantity(1.0, 'picosecond')> vs ime_per_iteration=<Quantity(1, 'picosecond')> Where the first one is what we define for the settings, and the second one is the default settings. BUT in the notebook we do time_per_iteration=1*unit.ps, yet when defining the defaults we do https://github.com/OpenFreeEnergy/openfe/blob/70379d7f0d875aa448e945e3d4ec65bcbd998e06/openfe/protocols/openmm_utils/omm_settings.py#L367

So I think if we want this assert to work, we need to change the default to be like time_per_iteration: FloatQuantity['picosecond'] = 1.0 * unit.picosecond instead of time_per_iteration: FloatQuantity['picosecond'] = 1 * unit.picosecond` so that the assert works.

I feel like this is something we spin out into an issue and just comment out the assert check in the cookbook for now