OpenFreeEnergy / openfe

The Open Free Energy toolkit
https://docs.openfree.energy
MIT License
141 stars 20 forks source link

Adaptive settings #974

Open jthorton opened 3 weeks ago

jthorton commented 3 weeks ago

Some transformations require non-standard settings like charge changes we need the CLI to automatically apply this to a network and a way to define the dynamical settings via a yaml settings file.

IAlibay commented 3 weeks ago

So the fix here might be mostly out of the CLI - what I would propose is revive the idea of passing ChemicalSystems to default settings. Then Protocols can set defaults depending on the transformation type.

That being said, it does make things a bit murky when you also have a YAML settings file, so maybe it requires a bit of discussion.

IAlibay commented 3 weeks ago

Here's the relevant gufe PR: https://github.com/OpenFreeEnergy/gufe/pull/282

jthorton commented 3 weeks ago

I think most of the CLI improvements should be exposed via some new API which the CLI just wraps around.

For the case of adaptive settings, I would propose a plugin system that allows users to write their own to maximise customisation as power users like Jenke at ASAP want to use different sampling times based on edge scoring. I would propose a base class with an abstract method which takes in a gufe.Transformation and then returns a new one with updated settings:

class SettingTransformer(abc.ABC):
    # settings here 
    @abc.abstractmethod
    def transform(transformation: gufe.Transformation) -> gufe.Transformation:
         do specific things here

We could then provide some basic ones like a charge change transformer which changes the number of lambda windows and simulation length, and users could then curate a list of SettingTransformers which are applied in order to an AlchemicalNetwork. This would also allow users an easy way to define the transformers which should be applied via a yaml file when using the CLI.

IAlibay commented 3 weeks ago

🤔 this would need more discussion - my initial take is that this breaks the model. I.e. Transformations are intended to not be something you go ahead and edit again after you create it.

The pattern really should be that you edit the settings to create a set of Protocols that are then applied based on a given criteria.

The hierarchy of dynamically assigning the right protocol to the right transformation is something you expect to be doing in a NetworkPlanner, which is currently thought to sit before the Transformation creation point.

jthorton commented 3 weeks ago

The pattern really should be that you edit the settings to create a set of Protocols that are then applied based on a given criteria.

I think that extending that would have a lot of branching logic that would become complicated over time as you add more conditions and you would be stopping experimentation by users if you had to hard code the conditions on when certain protocols are applied.

The hierarchy of dynamically assigning the right protocol to the right transformation is something you expect to be doing in a NetworkPlanner, which is currently thought to sit before the Transformation creation point.

I think that could be worked into my idea instead of taking a transform it takes the edge and the protocol settings and edits them and after passing through each object you would create the transformation

class SettingTransformer(abc.ABC):
    # settings here 
    @abc.abstractmethod
    def adapt(edge: gufe.LigandAtomMapping, protocol_settings: Settings) ->  Settings:
         do specific things here

for edge in ligand_network.edges:
    edge_settings = settings
    for transformer in transformers:
        edge_settings = transformer.adapt(edge, edge_settings)
    # now build the transform 
IAlibay commented 3 weeks ago

This is somethign that's best discussed in person - one major reasons for pushing things back to the Protocol is that the "default settings" and "how a Protocol should adapt" is probably a Protocol specific issue. I.e. you can have something that adapts settings outside of the Protocols, but once you start doing multiple Protocols, that becomes a bit nightmareish to keep up with (think of net charge as an example, if you had to keep track of net charge change settings for 5 different Protocols it'd become a problem).

RiesBen commented 2 weeks ago

@jthorton and @IAlibay

just as a comment, this could be an improvement in the AlchemicalNetwork Planners. You would combine here the getting the correct settings from the protocol with the AlchemicalNetworkPlanner implementations. :) These changes will be automagically appear in the CLI, as the CLI uses e.g. RBFEAlchemicalNetworkPlanner

(see here: https://github.com/OpenFreeEnergy/openfe/blob/main/openfe/setup/alchemical_network_planner/relative_alchemical_network_planner.py)