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

Adjust Gufe Abstractness in order to enable more protocols then RBFE with SmallMoleculeComponents #342

Open
RiesBen opened this issue Aug 27, 2024 · 3 comments · May be fixed by #346
Open
Assignees

Comments

@RiesBen
Copy link
Contributor

RiesBen commented Aug 27, 2024

The current gufe cufe for setting up FE networks (AlchemicalNetworks) is very specifically tailored for the RBFE protocol using SmallMoleculeComponents.
However this conceptual structure is not limited to that single use case and can be used for various approaches as well or even mixtures (portein mutations, sep-top, etc.).
Therefore we discussed this PR content and wanted to provide a more abstract layer to setup, in order to allow the development of new use cases for OpenFE and collaborators.

Here is the current workflow in setup:
RBFE_setup_pipeline drawio

Therefore here we introduce the following classes as discussed (pseudo definitions, to indicate the relations between the classes):

  • ComponentMapper(Component, Component)->ComponentMapping
    • A class that finds some kind of mapping between two molecules
    • this is currently LigandAtomMapper(SmallMoleculeComponent, SmallMoleculeComponent)->LigandAtomMapping, but this logic only works for hybridTopology approaches, wherease the structure is much more universal.
  • ComponentMappingScorer(ComponentMapping)->float
    • A class that evaluates a ComponentMapping
    • Currently we have here no class definition, but function signaures like: AtomMappingScorer(LigandAtomMapping)->float, but in the future a mapping less shape overlap comparison could be interesting for e.g. Sep-Top
  • NetworkPlanner(ComponentMapper, ComonentMappingScorer, Components)->NetworkPlan
    • Currently we have here no Class, however this changes with Konnektor. General function signature right now is (simplified): generate_maximal_network(ligands: Iterable[SmallMoleculeComponent], mappers: AtomMapper, scorer: Callable[[LigandAtomMapping], float]] = None,) -> LigandNetwork:
      but this process actually is totally independent of the type of components, mappers and scorers, as the algorithms life in an abstract graph world.
  • NetworkPlan:
    • currently this is represented by LigandNetwork, which again is very specific and does not represent the step in the process (plan) at all.

Part 2 (see issue: ):

  • AlchemicalNetworkPlanner
  • AbstractChemicalSystemGenerator
@RiesBen RiesBen changed the title Build ComponentMapping, ComponentMappingScorer Adjust Gufe Abstractness in order to enable more protocols then RBFE with SmallMoleculeComponents Oct 17, 2024
@atravitz
Copy link
Contributor

@dotsdl - what @RiesBen is saying here about LigandNetwork possibly becoming (or inheriting from) a new NetworkPlan class is similar to our conversation today.

Thoughts we had in offline conversation about limitations of LigandNetwork and potential solutions:
-LigandNetwork could inherit from a more general base class, likeNetworkPlan. We didn't fully think through all possible consequences of this, but it seems reasonable on its face

  • konnektor could create AlchemicalNetworks instead - this seems ill-advised, since it would creep konnektor out of "planning" territory and into "execution" territory. In general, we want to keep konnektor separate from actual execution details.

It looks like @RiesBen has already made progress on this and other item in #346, and I'm happy to pick it up if it's not within your plans before leaving openfe.

@IAlibay
Copy link
Member

IAlibay commented Oct 25, 2024

Just chiming in that Konnektor should definitely not be creating AlchemicalNetworks.

Re: NetworkPlan, I do agree with the idea of having a base class above LigandNetwork, my only caveat here is that the name NetworkPlan is not intuitive enough for that purpose. We do need to workshop a better name.

@atravitz
Copy link
Contributor

Yes for sure, it serves more as a counter-example.

I also agree we should be intentional with naming, I expect that PR will take more work to close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Planned
Development

Successfully merging a pull request may close this issue.

4 participants