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

Feature decouple somd 3 #29

Open
wants to merge 8 commits into
base: feature-decouple-somd-3
Choose a base branch
from

Conversation

Roy-Haolin-Du
Copy link
Contributor

Description

Replace template_config.cfg with Pydantic config

Todos

Remove the template_config.cfg and replace with a Pydantic SomdConfig class (which derived from a general EngineConfig class which can be configured in the code, or else read from a yaml/json file if not supplied.

Copy link
Collaborator

@fjclark fjclark left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! Just left a few comments.

self._stream_log_level = stream_log_level
self._set_up_logging()

def _set_up_logging(self) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can remove logging as it's not used. Thanks

config = self.get_config()
with open(file_path, "w") as f:
_yaml.safe_dump(config, f, default_flow_style=False)
self._logger.info(f"Configuration dumped to {file_path}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can probably remove, thanks

"""
pass

def __eq__(self, other: object) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to define if this derived from pdyantic base model.

from ..run._logging_formatters import _A3feStreamFormatter


class EngineRunnerConfig(ABC):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Inherit from base model please.


from ._engine_runner_config import EngineRunnerConfig as _EngineRunnerConfig


Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably shift the base class into the same file - have a think about it - up to you!

@@ -191,8 +192,8 @@ three replicates. Note that this is expected to produce an erroneously favourabl
cfg.runtime_npt = 50 # ps
cfg.ensemble_equilibration_time = 100 # ps
calc = a3.Calculation(ensemble_size = 3)
calc_set.setup(bound_leg_sysprep_config = cfg, free_leg_sysprep_config = cfg)
calc_set.run(adaptive = False, runtime=0.1) # ns
calc.setup(bound_leg_sysprep_config = cfg, free_leg_sysprep_config = cfg)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you!

@@ -140,7 +141,7 @@ Individual Simulation settings
-------------------------------

To customise the specifics of how each lambda window is run (e.g. timestep), you can use the ``set_simfile_option`` method. For example, to set the timestep to 2 fs, run
``calc.set_simfile_option("timestep", "2 * femtosecond")``. This will change parameters from the defaults given in ``template_config.cfg`` in the ``input`` directory, and warn
``calc.set_simfile_option("timestep", "2 * femtosecond")``. This will change parameters from the defaults generated by ``engine_config``, and warn
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to update this entire function to update the configs for all sub-simulation runners. Please rename it to something more appropriate e.g. set_config_option and test that it updates all configs.

@@ -304,12 +305,12 @@ You can run sets of calculations using the :class:`a3fe.run.CalcSet` class. To d
ABFE with Charged Ligands
*************************

Since A3FE 0.2.0, ABFE calculations with charged ligands are supported using a co-alchemical ion approach. The charge of the ligand will be automatically detected, assuming that this is correctly specified in the input sdf. The only change in the input required is that the use of PME, rather than reaction field electrostatics, should be specified in ``template_config.cfg`` as e.g.:
Since A3FE 0.2.0, ABFE calculations with charged ligands are supported using a co-alchemical ion approach. The charge of the ligand will be automatically detected, assuming that this is correctly specified in the input sdf. The only change in the input required is that the use of PME, rather than reaction field electrostatics, should be specified in ``somd_config.cfg`` as e.g.:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change to SomdEngineConfig (or whatever it was). Thanks

The default `somd_config.cfg` uses reaction field instead of PME. This is faster (around twice as fast for some of our systems) and has been shown to give equivalent results for neutral ligands in RBFE calculations - see https://pubs.acs.org/doi/full/10.1021/acs.jcim.0c01424 .
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please also change to point to the the config class rather than the template. Thanks!

@@ -338,19 +338,6 @@ def run(self, runtime: float = 2.5) -> None:
-------
None
"""
# Need to make sure that runtime is a multiple of the time per cycle
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please ensure that the somd.cfg file is written out before here, to ensure that the most up-to-date options are written out before the simulation is run.

@Roy-Haolin-Du
Copy link
Contributor Author

Hi Finlay,

I hope you're doing well!

So far, the template_config.cfg has been replaced with a Pydantic SomdConfig class, which can now be generated automatically when the simulation requires it.

Additionally, we aim to avoid writing the somd.cfg file until it’s ready to be submitted to Slurm. The only place where the file should be written is in the run method of the simulation.py file. The workflow should only interact with the somd_config object, avoiding read from file. Information should flow one way: from the Pydantic classes to the files, without reading data back from files.

To support this, I have removed all read_simfile_option calls and definitions, as well as the write_simfile_option function. However, the generated somd.cfg file is still missing some extended parameters, such as morph, top, crdFiles, and restraints.
Here is the related commit for reference: [8d7b957]
8d7b957.

Perhaps in our next meeting, we can discuss this part in detail and explore potential solutions. I’d appreciate any advice you have.

Thanks a lot for your support!

Best regards

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.

2 participants