-
Notifications
You must be signed in to change notification settings - Fork 25
Adaptive settings #974
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
Comments
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. |
Here's the relevant gufe PR: OpenFreeEnergy/gufe#282 |
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 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 |
🤔 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. |
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.
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 |
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). |
just as a comment, this could be an improvement in the AlchemicalNetwork Planners. |
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.
The text was updated successfully, but these errors were encountered: