Konnektor integration #927

RiesBen · 2024-09-03T06:50:57Z

This PR is integrating Konnektor to openFE which will offer in the future a default parallelization for all Network building and more different network tools.
In this PR there is one paradigm change currently present, leading to only being able to pass one Atom mapper to the functions.

Checklist

replace code with Konnektor implementations
- MST
- Maximal Network
- Star Map
- Redundant MST
- Explicit networks
Test we are getting the same results
- pytest suit - depends on adapting the Konnektor behavior to openfe. konnektor#84

Developers certificate of origin

I certify that this contribution is covered by the MIT License here and the Developer Certificate of Origin at https://developercertificate.org/.

pep8speaks · 2024-09-03T06:51:03Z

Hello @RiesBen! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-09-18 21:06:49 UTC

RiesBen · 2024-10-22T14:14:51Z

This PR should be ready for review to merge after this merge happened:
OpenFreeEnergy/konnektor#84

need to look into further other unit test failures.

jameseastwood · 2024-10-22T14:15:57Z

Please discuss with @atravitz and @hannahbaumann as part of the Konnektor handover plans.

jameseastwood · 2024-10-24T14:10:56Z

Needs follow-up discussion in a meeting planned for Oct. 28

IAlibay

Mostly blocking on API breaks - I agree that the name change may make sense in some cases, but these are very much user facing methods and we cannot break the API for 1.x, especially without some kind of deprecation cycle.

openfe/setup/ligand_network_planning.py

IAlibay · 2024-10-24T18:18:03Z

openfe/setup/ligand_network_planning.py

@@ -160,7 +167,7 @@ def generate_maximal_network(
    ----------
    ligands : Iterable[SmallMoleculeComponent]
      the ligands to include in the LigandNetwork
-    mappers : AtomMapper or Iterable[AtomMapper]
+    mapper : AtomMapper or Iterable[AtomMapper]


This docstring rename doesn't match the current signature (note that we can't rename from mappers to mapper without causing an API break).

@atravitz you marked this as resolved but the docstring still doesn't match - was this intentional?

openfe/setup/ligand_network_planning.py

atravitz · 2024-10-28T20:48:10Z

environment.yml

@@ -4,6 +4,7 @@ channels:
 dependencies:
  - duecredit<0.10
  - kartograf>=1.0.0
+  - konnektor


I think we need to make a Konnektor v0.2.0 release with this change, then pin this with konnektor>=v0.2.0.

openfe/setup/ligand_network_planning.py

RiesBen · 2024-10-29T22:39:28Z

openfe/tests/setup/test_network_planning.py

@@ -130,21 +130,7 @@ def test_radial_network_index_error(toluene_vs_others, lomap_old_mapper):
            ligands=ligands, central_ligand=2077,
            mappers=lomap_old_mapper, scorer=None,
        )
-
-
-def test_radial_network_self_central(toluene_vs_others, lomap_old_mapper):


This can not happen anymore with the current Konnektor and OpenFE implementation.
I propose removing the test.

@IAlibay is there a concern about breaking backwards compatibility here?

By "this cannot happen" do we mean that Konnektor just gracefully handles things such that the network cannot end up with a self edge?

If so, then my suggestion is to keep the code that throws a warning?

If it's more "if you try to do that Konnektor will error", then we should retain the behaviour that pops the ligand out and throws the warning, with a deprecation warning that this behaviour will go away in 2.0.

RiesBen · 2024-10-29T23:06:48Z

I went further through the current CI problems and hope I caught all problems for this PR (@atravitz):

*unreachable unit tests: there a error messages missing. -> We need to check why this error is not appearing. This should be thrown at the Konnektor level.
generate maximal network: the unitest behavior expects each edge direction leading to 56 edges. I assume it is either due to directionality of edges or number of possible mappings? However, Konnektor considers edges as non-directed at the current time point and allows only on possible mapper solution == one edge. Therefore it returns 28 edges. Other approaches might lead to problems with the graph algorithms. -> I would propose to adjust the edge number to 28. After verifying the behavior reason.
redundant mst: generates one edge to less then expected? (expected 14 (2*(n-1)), got 13) -> need to look into why this is.
explicit networks: The problem here is image you get a list of 8 ligands and 2 defined edges. Currently only the connected part is returned as a ligand network, so two edges with 3 nodes. That clashes with the current unit tests, expecting 3 edges and 8 nodes. This translates to the expectation of getting back a disconnected network as a default. I'm not sure why here it was allowed to generate disconnected networks. -> I would propose to throw an error in these cases in order to avoid user confusion.

fix for tests

- fix removing the central ligand from list.

remove test for testing if a central component is part of the other component list. This can not happen anymore with the current Konnektor implementation, therefore I propose removing the test.

This reverts commit bf82085.

…ch konnektor

IAlibay

partial review

IAlibay · 2024-11-18T13:47:04Z

openfe/setup/ligand_network_planning.py

    mappers: Union[AtomMapper, Iterable[AtomMapper]],
+    central_ligand: Union[SmallMoleculeComponent, str, int, None],


I wouldn't change the order unless we really have to - i.e. it's technically API breaking in the case that someone just passes arguments without specifying them?

IAlibay · 2024-11-18T13:48:37Z

openfe/setup/ligand_network_planning.py

@@ -160,7 +167,7 @@ def generate_maximal_network(
    ----------
    ligands : Iterable[SmallMoleculeComponent]
      the ligands to include in the LigandNetwork
-    mappers : AtomMapper or Iterable[AtomMapper]
+    mapper : AtomMapper or Iterable[AtomMapper]


@atravitz you marked this as resolved but the docstring still doesn't match - was this intentional?

IAlibay · 2024-11-18T14:09:12Z

openfe/setup/ligand_network_planning.py

-    edges = []
-
-    for ligand in ligands:
-        if ligand == central_ligand:


What happens in the new code if you pass in multiple copies of the same central ligand in the ligand list?

IAlibay · 2024-11-18T14:09:41Z

openfe/setup/ligand_network_planning.py

@@ -160,7 +171,7 @@ def generate_maximal_network(
    ----------
    ligands : Iterable[SmallMoleculeComponent]
      the ligands to include in the LigandNetwork
-    mappers : AtomMapper or Iterable[AtomMapper]
+    mapper : AtomMapper or Iterable[AtomMapper]


revert to mappers?

RiesBen linked an issue Sep 11, 2024 that may be closed by this pull request

Konnektor Integration to OpenFE OpenFreeEnergy/konnektor#17

Open

4 tasks

RiesBen removed a link to an issue Sep 18, 2024

Konnektor Integration to OpenFE OpenFreeEnergy/konnektor#17

Open

4 tasks

RiesBen added enhancement New feature or request networkplanning labels Sep 18, 2024

jameseastwood assigned RiesBen Oct 21, 2024

RiesBen changed the title ~~[WIP] - Konnektor integration~~ Konnektor integration Oct 22, 2024

jameseastwood assigned atravitz and hannahbaumann Oct 24, 2024

IAlibay requested changes Oct 24, 2024

View reviewed changes

IAlibay added the priority:medium label Oct 28, 2024

atravitz requested changes Oct 28, 2024

View reviewed changes

RiesBen commented Oct 29, 2024

View reviewed changes

jameseastwood unassigned RiesBen Oct 30, 2024

jameseastwood mentioned this pull request Nov 6, 2024

Konnektor - Integration #908

Closed

atravitz force-pushed the KonnektorIntegration branch 3 times, most recently from 1205c63 to 46986af Compare November 6, 2024 20:01

RiesBen added 8 commits November 15, 2024 09:46

draft for Konnektor integration

569895a

draft for Konnektor integration

f73dea5

changing back to mappers iterable as parameter

85f2ff4

consistently using Konnektor also in loading fep or orion net

eab3238

adding handles for Konnektor functionality.

639ef19

used black for formatting.

d36d37d

used black for formatting.

2e0878f

Update environment.yml

92c5b7c

RiesBen and others added 10 commits November 15, 2024 09:46

Update test_network_planning.py

6a37703

fix for tests

fixes for unittests:

be12620

- fix removing the central ligand from list.

Update test_network_planning.py

6ef48f7

remove test for testing if a central component is part of the other component list. This can not happen anymore with the current Konnektor implementation, therefore I propose removing the test.

updating docstrings

92fc868

adjusting n_processes docstring language

609ddc6

fixing central_ligand default

69eea0d

rename mappers to mapper to avoid api break

db191f4

Revert "rename mappers to mapper to avoid api break"

7f0cfe1

This reverts commit bf82085.

fixing mapper/s to avoid api break

144d345

switching arg names from 'ligands' to new 'components' keyword to mat…

6d3e59a

…ch konnektor

atravitz force-pushed the KonnektorIntegration branch from 24a7abd to 6d3e59a Compare November 15, 2024 17:46

IAlibay requested changes Nov 18, 2024

View reviewed changes

This was referenced Nov 18, 2024

resolve konnektor behavior for maximal network generation #1019

Open

redundant mst generates one fewer edge than openfe expects OpenFreeEnergy/konnektor#96

Open

resolve disconnected network handling #1020

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Konnektor integration #927

Konnektor integration #927

RiesBen commented Sep 3, 2024 •

edited

Loading

pep8speaks commented Sep 3, 2024 •

edited

Loading

RiesBen commented Oct 22, 2024 •

edited

Loading

jameseastwood commented Oct 22, 2024

jameseastwood commented Oct 24, 2024

IAlibay left a comment

IAlibay Oct 24, 2024

IAlibay Nov 18, 2024

atravitz Oct 28, 2024

RiesBen Oct 29, 2024

atravitz Nov 6, 2024

IAlibay Nov 7, 2024

RiesBen commented Oct 29, 2024 •

edited

Loading

IAlibay left a comment

IAlibay Nov 18, 2024

IAlibay Nov 18, 2024

IAlibay Nov 18, 2024

IAlibay Nov 18, 2024

		mappers: Union[AtomMapper, Iterable[AtomMapper]],
		central_ligand: Union[SmallMoleculeComponent, str, int, None],

Konnektor integration #927

Are you sure you want to change the base?

Konnektor integration #927

Conversation

RiesBen commented Sep 3, 2024 • edited Loading

Developers certificate of origin

pep8speaks commented Sep 3, 2024 • edited Loading

Comment last updated at 2024-09-18 21:06:49 UTC

RiesBen commented Oct 22, 2024 • edited Loading

jameseastwood commented Oct 22, 2024

jameseastwood commented Oct 24, 2024

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RiesBen commented Oct 29, 2024 • edited Loading

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RiesBen commented Sep 3, 2024 •

edited

Loading

pep8speaks commented Sep 3, 2024 •

edited

Loading

RiesBen commented Oct 22, 2024 •

edited

Loading

RiesBen commented Oct 29, 2024 •

edited

Loading