Feature decouple somd 3 #40

Roy-Haolin-Du · 2025-03-05T11:07:49Z

Description

Fixbug for reading the correct slurm_out_file

EngineType enmu changed and remove the mapping dictionaries.

Correct tmp_files name and make sure lam values enable setting from engine_config

Integration test run locally, won't run on CI

Close #11

Status

All tests and format check passed
Run non-adaptive/adaptive a3fe cases successfully

Ready to go

Hi Finlay, hope you are well!
Could you please review @fjclark ?

…e-somd-3

…onfig

fjclark

Thanks so much Roy, this looks great! I especially like the thorough tests and what you've done with the integration tests is really nice. Having a general engine_type fixture is a neat solution.

I've left a few small comments to address. A few things left to do before we merge into main are:

Add extensive docs
Possibly add a few more tests on e.g. updating config options at the calculation level and checking that the changes propagate down to the simulation level
Possibly adding a makefile (see https://github.com/fjclark/red/blob/main/Makefile) to simplify installation and add pre-commit hooks (something like https://github.com/fjclark/red/blob/main/.pre-commit-config.yaml) to prevent us forgetting to format before we commit - I always do this. However, this isn't essential, so we can always do this later if you'd prefer.

fjclark · 2025-03-08T09:35:38Z

.github/workflows/CI.yaml

@@ -76,7 +76,8 @@ jobs:
        # conda setup requires this special shell
        shell: bash -l {0}
        run: |
-          pytest -v --cov=a3fe --cov-report=xml --color=yes a3fe/tests/
+          # use -m "not integration" to exclude all integration tests
+          pytest -v --cov=a3fe --cov-report=xml --color=yes a3fe/tests/ -m "not integration"


Nice! Clean solution.

a3fe/__init__.py

a3fe/analyse/mbar.py

a3fe/configuration/__init__.py

a3fe/configuration/engine_config.py

a3fe/tests/test_run_integration.py

fjclark · 2025-03-08T10:47:29Z

a3fe/tests/test_run_integration.py

+
+        calc.get_optimal_lam_vals(delta_er=2)
+
+        # Check that the old runs have been moved, and that the new lambda values are


Please could you check that the new lambda values are different to the old ones? Thanks!

fjclark · 2025-03-08T10:48:46Z

a3fe/tests/test_run_integration.py

+        # Check that the calculation is not running
+        assert not calc.running
+
+        # Set the entire simulation time to equilibrated for analysis


Please don't set the equilibration time - this should be done automatically when running adaptively. Please check that the equilibration time has successfully been set. Thanks

fjclark · 2025-03-08T10:49:20Z

a3fe/tests/test_run_integration.py

+        assert not calc.running
+
+        # Set the entire simulation time to equilibrated for analysis
+        for leg in calc.legs:


I used to have to do this before calc.set_equil or whatever it is was introduced. Please use this function instead.

a3fe/tests/test_run_integration.py

…ccess them with a3.SomdConfig etc

….Calculation

… propagate to simulation level

…onfig hooks

Roy-Haolin-Du · 2025-03-21T10:11:38Z

Description

Make the test functions specific to SOMD
Make sure the integrations test (non-) adaptive run working
Add extensive docs to emphasize updates (pkl changed to yaml), Makefile, pre commit hooks.

All tests and format check passed
Run non-adaptive/adaptive a3fe cases successfully

Hi Finlay, hope you are well!
Could you please review @fjclark ?
No rush! Just take time when you are free.

Thanks so much.

fjclark

Looking great Roy! Thanks very much for this. I've left a few comments - sorry for being so pedantic. However, this is super close to being merge-able. The updates to the docs look especially great and are super clear.

fjclark · 2025-03-24T06:41:40Z

.pre-commit-config.yaml

+        entry: bash
+        args: [-c, "make lint"]
+
+    # type check hook


This overall looks great! However, I'd probably skip the mypy hook for now - at least when I try it, I get hundreds of mypy errors because I didn't start the project with strict type checking and it would be a lot of work to fix it now (although we should probably fix this at some point). The type hints are still useful for understanding the code and for vscode to spot basic mistakes.

fjclark · 2025-03-24T06:41:40Z

.pre-commit-config.yaml

+        entry: bash
+        args: [-c, "make lint"]
+
+    # type check hook


This overall looks great! However, I'd probably skip the mypy hook for now - at least when I try it, I get hundreds of mypy errors because I didn't start the project with strict type checking and it would be a lot of work to fix it now (although we should probably fix this at some point). The type hints are still useful for understanding the code and for vscode to spot basic mistakes.

fjclark · 2025-03-24T06:46:46Z

Makefile

+# For the CI github actions workflow, we skip "make env" and set up the environment manually. In this case,
+# it's helpful to to set CONDA_ENV_RUN to be empty. However, for the documentation workflow, we want to override
+# this and keep the normal behavior. We override this by setting KEEP_CONDA_ENV_RUN to true in the documentation workflow.
+SKIP_CONDA_ENV = $(and $(GITHUB_ACTIONS),$(if $(KEEP_CONDA_ENV_RUN),,true))


Thanks, looks great!

I can't quite remember why I added this logic (it's super convoluted) but I think we might be able to simplify here - red is designed to work on windows/ mac/ linux, which makes the CI more complicated. We only test a3fe on linux because we need slurm, which is linux only. So I think you might be able to remove this complicated logic and just use the make env etc commands as done e.g. here https://github.com/SimonBoothroyd/absolv/blob/main/.github/workflows/ci.yaml - nice and simple.

fjclark · 2025-03-24T06:47:17Z

Makefile

+	$(CONDA_ENV_RUN) ruff format $(PACKAGE_DIR)
+	$(CONDA_ENV_RUN) ruff check --fix --select I $(PACKAGE_DIR)
+
+# run tests


Could you please add a test-integration command to run the integration tests? Thanks!

fjclark · 2025-03-24T06:48:15Z

Makefile

+	$(CONDA_ENV_RUN) pytest $(TEST_ARGS) $(PACKAGE_DIR)/tests/
+
+# type check
+type-check:


As mentioned above, when I run the type checking I get hundreds of errors - maybe best to remove this for now (as fixing would be a bit of a pain right now)

docs/guides.rst

fjclark · 2025-03-24T08:02:20Z

a3fe/tests/__init__.py

-elif os.environ.get("RUN_SLURM_TESTS") is None:
-    # Default to not running SLURM tests unless explicitly enabled
-    RUN_SLURM_TESTS = False
+RUN_SLURM_TESTS = os.environ.get("RUN_SLURM_TESTS") not in [None, "False"]


Ah, should probably add 0 to this list (as in RUN_SLURM_TESTS=0 should avoid running the tests, and RUN_SLURM_TESTS=1 should run them)! Sorry I forgot

…unction

Roy-Haolin-Du · 2025-04-14T08:28:55Z

Description

Simplify env setup in Makefile, pre-commit-hooks and CI configuration
Refactor calc.setup : sysprep_config setting both bound and free leg
Added extensive docs and modified all test files to make sure meeting requirements and working

All tests and format check passed
Run non-adaptive/adaptive a3fe cases successfully

Hi Finlay, hope you are well.
I know you’ve got a busy week with the group meeting and your viva,
Just take a look whenever you have a moment please, no rush!
@fjclark

Thanks so much.

fjclark · 2025-05-20T11:12:25Z

.github/workflows/CI.yaml

        run: |
-          python -m pip install . --no-deps
-          micromamba list
+          export PACKAGE_NAME=a3fe


Could you just run "make env" here - this seems to be the same as in the makefile. Sorry if I'm missing something!

fjclark · 2025-05-20T11:14:16Z

.pre-commit-config.yaml

-
-    # type check hook
-    -   id: mypy
-        name: Type check python code


It would be nice to sort out the typing sometime, but definitely one for the future. Apologies for not doing the typing properly from the start!

fjclark · 2025-05-20T11:15:50Z

Makefile

-	# Install development tools
-	$(CONDA_ENV_RUN) pip install -e .
+	mamba create -y --name $(PACKAGE_NAME)
+	mamba env update --name $(PACKAGE_NAME) --file devtools/conda-envs/test_env.yaml


Thanks, this is nice and simple

fjclark · 2025-05-20T11:18:43Z

Makefile

+	$(error VERSION is not set)
+endif
+	$(CONDA_ENV_RUN) mike deploy --push --update-aliases $(VERSION)


Nice, can the docs now be deployed with mike? That's great if we now have versioned docs - before I did it badly so only the last docs were accessible.

fjclark · 2025-05-20T11:20:37Z

a3fe/run/calc_set.py

@@ -156,19 +156,15 @@ def calcs(self, value: _Calculation) -> None:

    def setup(
        self,
-        bound_leg_sysprep_config: _Optional[_BaseSystemPreparationConfig] = None,
-        free_leg_sysprep_config: _Optional[_BaseSystemPreparationConfig] = None,
+        sysprep_config: _Optional[_BaseSystemPreparationConfig] = None,


Thanks, I think it's cleaner with just the one config!

fjclark · 2025-05-20T11:28:05Z

a3fe/run/calculation.py

@@ -143,7 +141,7 @@ def _validate_input(self) -> None:
        # Check backwards, as we care about the most advanced preparation stage
        for prep_stage in reversed(_PreparationStage):
            files_absent = False
-            for leg_type in Calculation.required_legs:
+            for leg_type in [_LegType.FREE, _LegType.BOUND]:


Could we avoid hard-coding this? I'm imagining the case where someone only wants to run e.g. the free leg for some reason. I know that it's hard because we don't yet have the sys prep config, which would contain this information. Some options: 1) supply the sys prep config to init instead of setup 2) Only validate when someone attempts to run setup. What do you think?

fjclark · 2025-05-20T11:31:32Z

a3fe/tests/README.md

-```
+   ```bash
+   cd your/path/a3fe
+   RUN_SLURM_TESTS=1 pytest a3fe/tests --run-integration -v


Maybe you could also mention they can now run "make test-integration" - thanks for adding that!

fjclark · 2025-05-20T11:36:33Z

a3fe/tests/README.md

   ```bash
-   RUN_SLURM_TESTS=1 pytest a3fe/tests/test_run_integration.py::TestSlurmIntegration::test_slurm_calculation_setup --run-integration -v
+   RUN_SLUEM_TESTS=0 pytest a3fe/tests --run-integration -v


Small typo "SLUEM". Also, should --run-integration be removed to avoid confusion? My understanding is that this says "run the integration tests" but RUN_SLURM_TESTS=0 stops them running. In the future, these commands might do different things.

fjclark · 2025-05-20T11:43:06Z

a3fe/tests/test_run_integration.py

+            for stage_name in LEGS_WITH_STAGES[leg_name]:
+                # Check that the check_equil_multiwindow_paired_t.txt file exists
+                leg_path = os.path.join(calc.base_dir, leg_name)
+                stage_path = os.path.join(leg_path, stage_name)


This looks great. Just commenting on something I've done badly - I've mixed the using strings as paths with os.path.join and using pathlib.Path throughout the code. I think pathlib.Path is nicer and if I wrote from scratch I would only use pathlib.Path. Just mentioning for awareness though - this isn't something we really need to fix.

fjclark · 2025-05-20T11:47:17Z

docs/CHANGELOG.rst

 - Separated system preparation configuration from system preparation and configured SOMD with a3fe.configuration.system_prep_config.SomdSystemPreparationConfig.

+**Migration Guide**:
+In previous versions, users needed to configure run_somd.sh and template_config.cfg files to run calculations. 
+However, with version 0.4.0, these files are no longer needed. Instead, configuration objects (SlurmConfig, SomdConfig, and SomdSystemPreparationConfig) are passed directly to the Calculation object.


Brilliant, thanks! Maybe add a link to where the docs show an example of doing this?

fjclark · 2025-05-20T11:54:09Z

docs/guides.rst

    calc = a3.Calculation(engine_config=engine_config) # Pass to Calculation during creation

    # Or modify parameters after creating the Calculation
    calc = a3.Calculation()
-    calc.engine_config.timestep = 2.0
+    # Works if the calculation has not been setup yet
+    calc.engine_config.timestep = 2.0 # fs


Thanks very much for pointing this out in the docs - this is a super important point. I feel like this may still cause some confusion if people don't read the docs properly. I wonder if we could warn the user if they edit the config directly when the calc has already been setup? Do you have any ideas? Don't worry if not as it's already way better than it used to be! This is fine, just trying to minimise confusion. Might be worth reminding users why this is required - just saying because the configs for the legs etc also need updated

fjclark · 2025-05-20T11:55:10Z

docs/guides.rst


 To see a complete list of available configuration options, run ``somd-freenrg --help-config``.
+or inspect the ``a3.SomdConfig`` class in the code.


Could maybe link to the API reference for this class

fjclark · 2025-05-20T11:56:05Z

docs/guides.rst

+    calc.setup(sysprep_config=cfg)
+
+You can also modify ``lambda values`` in the config object and the system will only run this defined leg and stage.
+For example, to only run the bound restrain stage with specified lambda values, set:


Nice, super clear and helpful

fjclark · 2025-05-20T12:15:46Z

Hi @Roy-Haolin-Du,

Thanks very much for the changes, and sorry it took me so long to review - have been super busy recently. This looks great and I've just left a couple of very small and pedantic suggestions. It's pretty much ready to go! I'd suggest this route to release, if good with you:

You address the minor changes above, when you have time and if you agree with the comments
I'll double-check your changes, then have a quick play around with the new code on ishikawa, when you have some time between calculations - if that's ok (I don't have access to any SLURM workstations at the moment)?
Merge and release!
You create a zenodo doi for the release including you as an author and stick it on the readme so that people can cite you if they use the code

Does that sound good?

Sorry again for the mega delay, and apologies in advance for more slow replies - I've got quite a few holidays and other work on til mid June, but should be less busy then!

Roy-Haolin-Du added 9 commits February 27, 2025 17:31

Make sure get_result function reads the Scorrect surm output file

2aed196

ruff check for consistent file formatting

81819f1

changed the EngineType enum, remove the mapping dictionaries

a47a53e

Test the EngineType enum

2a2c1c4

Merge branch 'michellab:feature-decouple-somd-3' into feature-decoupl…

833ab6a

…e-somd-3

Correct the tmp_files name in analyse

aaf595d

Enable lam_vals to be set when lambda values are passed from engine_c…

d51eccd

…onfig

Add integrations tests to be run locally, but won't be run on the CI

cf3fc80

ruff formating for all files

d90398e

Roy-Haolin-Du requested a review from fjclark March 5, 2025 11:09

fjclark requested changes Mar 8, 2025

View reviewed changes

Roy-Haolin-Du added 7 commits March 12, 2025 16:47

Still import SomdConfig and SomdSystemPreparationConfig so that can a…

d106e89

…ccess them with a3.SomdConfig etc

remove comments and redundant copy

50b2281

Make the test functions specific to SOMD and pass engine_type into a3…

f1a80c4

….Calculation

updating config options at calculation level and checking the changes…

320e6d4

… propagate to simulation level

Make sure integrations test (non-)adapative run working

57d1cdd

ruff formating for all files

a2826d3

Some extensive docs on new features. Adding Makefile and pre-commit-c…

81b5021

…onfig hooks

fjclark requested changes Mar 24, 2025

View reviewed changes

Roy-Haolin-Du added 6 commits March 27, 2025 12:01

Simplied env setup in Makefile and CI configuration

954c8b2

Extensive docs on new features and simplify pre-commit hooks

9e46e01

refactor calc.setup: sysprep_config setting both bound and free legs

19b16bb

update engine config at top level using update_engine_config_option f…

d7a1829

…unction

check the exact expected equil time is found in adaptive test

7ed93fe

Using a small system integration test convenient

6864a47

fjclark reviewed May 20, 2025

View reviewed changes


		calc.get_optimal_lam_vals(delta_er=2)

		# Check that the old runs have been moved, and that the new lambda values are


		To see a complete list of available configuration options, run ``somd-freenrg --help-config``.
		or inspect the ``a3.SomdConfig`` class in the code.

Feature decouple somd 3 #40

Are you sure you want to change the base?

Feature decouple somd 3 #40

Uh oh!

Conversation

Roy-Haolin-Du commented Mar 5, 2025

Description

Status

Uh oh!

fjclark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Roy-Haolin-Du commented Mar 21, 2025

Uh oh!

fjclark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Roy-Haolin-Du commented Apr 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjclark commented May 20, 2025

Uh oh!

Uh oh!