85 review docstrings #86

atravitz · 2024-10-25T14:38:54Z

Some typo and grammar fixes, as well as some questions.

codecov · 2024-10-25T14:42:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.81%. Comparing base (8c62d86) to head (b66f0e1).

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #86   +/-   ##
=======================================
  Coverage   86.81%   86.81%           
=======================================
  Files          49       49           
  Lines        1577     1577           
=======================================
  Hits         1369     1369           
  Misses        208      208

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/konnektor/network_planners/concatenators/cyclic_concatenator.py

RiesBen · 2024-10-29T23:15:10Z

src/konnektor/network_planners/generators/clustered_network_generator.py

@@ -40,7 +40,7 @@ def __init__(
        clusterer: ComponentsDiversityClusterer = ComponentsDiversityClusterer(
            featurize=RDKitFingerprintTransformer(), cluster=KMeans(n_clusters=3)
        ),
-        mappers: Union[AtomMapper, list[AtomMapper]] = None,
+        mappers: Union[AtomMapper, list[AtomMapper]] = None,  # include None in this union?


I confess, I really don't know what the correct type annotation is if there is a default None. :(

You can simply do Union[AtomMapper,list[AtomMapper],None]

However, it's not optional in the parent class, so I don't think it should be optional here.

I agree. I don't understand if there are cases where you would want to allow not passing through an AtomMapper. Also the score, line below, is optional, while not optional in the parent class. Lines 98 and 106 would be the cases where you don't need the AtomMapper and scorer, but I don't understand yet what those would do. I have to look into this in more detail.

hm, it actually seems like mappers are not required in the parent's parent class, NetworkPlanner. I opened an issue to resolve this, since this is no longer a docstrings edit: #93

RiesBen · 2024-10-29T23:16:51Z

src/konnektor/network_planners/concatenators/mst_concatenator.py

@@ -49,15 +49,15 @@ def __init__(
            scorer=scorer,
            network_generator=MstNetworkAlgorithm(),
            n_processes=n_processes,
-            _initial_edge_lister=None,
+            _initial_edge_lister=None,  ## TODO: should this be _initial_edge_lister?


I'm not sure if i understand this comment correctly, as before and after looks the same?

Suggested change

_initial_edge_lister=None, ## TODO: should this be _initial_edge_lister?

_initial_edge_lister=_initial_edge_lister,

I'm asking if _initial_edge_lister should be passed through here, right now this is forcing _initial_edge_lister=None

It looks like that in the parent class, the _initial_edge_lister=None, however, in the doc string it says

_initial_edge_lister: LigandNetworkPlanner, optional this LigandNetworkPlanner is used to give the initial set of edges. For standard usage, the Maximal NetworPlanner is used. However in large scale approaches, it might be interesting to use the heuristicMaximalNetworkPlanner. (default: None)

In this class MSTConcatenator it is listed as _initial_edge_lister: NetworkConcatenator = None,, while I think it should be a LigandNetworkPlanner? Or are those two connected?

In lines 85-89, all possible edges between the networks are created, therefore I would think that the initial_edge_lister is not needed, so None could be ok?!

it is valid for it to be None, but the issue here is that if _initial_edge_lister is defined in the MstConcatenator class, it never gets passed to the super()__init__() call.

How should we go about this, should we raise an issue and address this in a separate PR?

I agree with @atravitz - if _initial_edge_lister is part of the signature it should be passed through instead of hard-coding to None. The argument should also be listed in the class docstring.

Furthermore, we should avoid "private" kwargs in a user-facing class method (i.e. anything starting with a "_" that's bad form imho).

Fixing in a separate issue/PR combo is fine, especially since it seems like we'll need to rename the variable.

atravitz

@hannahbaumann could you take a look at these last two open conversations?

atravitz · 2024-11-08T21:54:11Z

src/konnektor/network_planners/concatenators/mst_concatenator.py

@@ -49,15 +49,15 @@ def __init__(
            scorer=scorer,
            network_generator=MstNetworkAlgorithm(),
            n_processes=n_processes,
-            _initial_edge_lister=None,
+            _initial_edge_lister=None,  ## TODO: should this be _initial_edge_lister?


Suggested change

_initial_edge_lister=None, ## TODO: should this be _initial_edge_lister?

_initial_edge_lister=_initial_edge_lister,

I'm asking if _initial_edge_lister should be passed through here, right now this is forcing _initial_edge_lister=None

atravitz · 2024-11-08T21:57:57Z

src/konnektor/network_planners/generators/clustered_network_generator.py

@@ -40,7 +40,7 @@ def __init__(
        clusterer: ComponentsDiversityClusterer = ComponentsDiversityClusterer(
            featurize=RDKitFingerprintTransformer(), cluster=KMeans(n_clusters=3)
        ),
-        mappers: Union[AtomMapper, list[AtomMapper]] = None,
+        mappers: Union[AtomMapper, list[AtomMapper]] = None,  # include None in this union?


You can simply do Union[AtomMapper,list[AtomMapper],None]

However, it's not optional in the parent class, so I don't think it should be optional here.

hannahbaumann

Here my comments on the two open questions, will go through the rest later. Maybe we can also meet and discuss @atravitz , I'm also not fully sure yet.

hannahbaumann · 2024-11-13T10:45:08Z

src/konnektor/network_planners/concatenators/mst_concatenator.py

@@ -49,15 +49,15 @@ def __init__(
            scorer=scorer,
            network_generator=MstNetworkAlgorithm(),
            n_processes=n_processes,
-            _initial_edge_lister=None,
+            _initial_edge_lister=None,  ## TODO: should this be _initial_edge_lister?


It looks like that in the parent class, the _initial_edge_lister=None, however, in the doc string it says

_initial_edge_lister: LigandNetworkPlanner, optional this LigandNetworkPlanner is used to give the initial set of edges. For standard usage, the Maximal NetworPlanner is used. However in large scale approaches, it might be interesting to use the heuristicMaximalNetworkPlanner. (default: None)

In this class MSTConcatenator it is listed as _initial_edge_lister: NetworkConcatenator = None,, while I think it should be a LigandNetworkPlanner? Or are those two connected?

In lines 85-89, all possible edges between the networks are created, therefore I would think that the initial_edge_lister is not needed, so None could be ok?!

hannahbaumann · 2024-11-13T14:35:14Z

src/konnektor/network_planners/concatenators/mst_concatenator.py

@@ -39,7 +39,7 @@ def __init__(
            scoring function evaluating an atom mapping, and giving a score
            between [0,1].
        n_connecting_edges: int, optional
-            number of connecting edges. (default: 2)
+            maximum number of connecting edges. (default: 2)


Are there cases where not exactly n_connecting_edges are being generated?

Yes, Kruskal's algorithm will not create a cycle, so it iteratively adds edges up until either its criteria is met, or it reaches the user-provided number of connecting edges. I think we should rename this to max_edges or similar, but that's out of the scope of this PR.

We can rename pre-1.0, so please raise an issue for this!

hannahbaumann · 2024-11-13T14:37:00Z

src/konnektor/network_planners/generators/_abstract_network_generator.py

            the AtomMappers to use to propose mappings.  At least 1 required,
            but many can be given, in which case all will be tried to find the
            lowest score edges
        scorer : AtomMappingScorer
-            any callable which takes a AtomMapping and returns a float
+            Any callable which takes a AtomMapping and returns a float
+        network_generator:


Do we need to add the explanation here? (I'm not yet sure what the AbstractNetworkAlgorithm is, will have to check).

this naming convention is confusing, I think we should rename this as a follow-up

src/konnektor/network_planners/generators/cyclic_network_generator.py

src/konnektor/network_planners/generators/maximal_network_generator.py

src/konnektor/network_planners/generators/redundant_minimal_spanning_tree_network_generator.py

Co-authored-by: Hannah Baumann <43765638+hannahbaumann@users.noreply.github.com>

…class

… parent class" This reverts commit ae0973b.

IAlibay

I'm going to approve as "this is an improvement", but there are follow-up work items that need to be dealt with. For example there's a ton of old references to Transformation that are likely innapropriate.

IAlibay · 2024-11-20T12:35:31Z

src/konnektor/network_planners/concatenators/mst_concatenator.py

@@ -39,7 +39,7 @@ def __init__(
            scoring function evaluating an atom mapping, and giving a score
            between [0,1].
        n_connecting_edges: int, optional
-            number of connecting edges. (default: 2)
+            maximum number of connecting edges. (default: 2)


We can rename pre-1.0, so please raise an issue for this!

IAlibay · 2024-11-20T12:40:55Z

src/konnektor/network_planners/concatenators/mst_concatenator.py

@@ -49,15 +49,15 @@ def __init__(
            scorer=scorer,
            network_generator=MstNetworkAlgorithm(),
            n_processes=n_processes,
-            _initial_edge_lister=None,
+            _initial_edge_lister=None,  ## TODO: should this be _initial_edge_lister?


I agree with @atravitz - if _initial_edge_lister is part of the signature it should be passed through instead of hard-coding to None. The argument should also be listed in the class docstring.

Furthermore, we should avoid "private" kwargs in a user-facing class method (i.e. anything starting with a "_" that's bad form imho).

Fixing in a separate issue/PR combo is fine, especially since it seems like we'll need to rename the variable.

IAlibay · 2024-11-20T12:42:00Z

src/konnektor/network_planners/generators/_abstract_network_generator.py

@@ -24,32 +24,34 @@ def __init__(
        self,
        mappers: Union[AtomMapper, list[AtomMapper]],
        scorer,
+        ## TODO: rename this to network_algorithm?


We'd have to consider what this means to the class names everywhere else, but I agree that it would make sense.

IAlibay · 2024-11-20T12:43:45Z

src/konnektor/network_planners/generators/_abstract_network_generator.py

@@ -24,32 +24,34 @@ def __init__(
        self,
        mappers: Union[AtomMapper, list[AtomMapper]],
        scorer,


Probably should type hint these with Callable?

IAlibay · 2024-11-20T12:46:27Z

src/konnektor/network_planners/generators/maximal_network_generator.py

+        The `Transformation` s of this graph are realized as `AtomMapping` s of pairwise `Component` s.
+        If not all mappings can be created, it will ignore the mapping failure and return a nearly fully connected graph.
+
+        Note: This approach is not recommended for Free Energy calculations in application cases, as it is


How about using the ... note:: directive here?

IAlibay · 2024-11-20T12:47:26Z

src/konnektor/network_planners/generators/maximal_network_generator.py

-        > **Note**: the `MaximalNetworkGenerator` is parallelized and the number of CPUs can be given with  `n_processes`.
+        The `MaximalNetworkGenerator` builds a fully connected graph for given set of `Component` s.
+        It assumes each `Component` can be connected to every other `Component`.
+        The `Transformation` s of this graph are realized as `AtomMapping` s of pairwise `Component` s.


Maybe a separate issue, but we really need to avoid talking about Transformation especially capitalized.

This effectively refers to a gufe.Transformation, which Konnektor should have no knowledge of.

IAlibay · 2024-11-20T12:48:01Z

src/konnektor/network_planners/generators/maximal_network_generator.py

+        then remove edges to achieve the desired design.
+
+        This class is recommended as an `initial_edge_lister` for other approaches.
+        > **Note**: the `MaximalNetworkGenerator` is parallelized and the number of CPUs can be given with `n_processes`.


Why is this note differently formatted than the other note?

IAlibay · 2024-11-20T12:48:31Z

src/konnektor/network_planners/generators/minimal_spanning_tree_network_generator.py

-        The `MinimalSpanningTreeNetworkGenerator`, builds an minimal spanning tree (MST) network for a given set of `Component` s. The `Transformation` s of the Network,
-        are represented by an `AtomMapping` s, which are scored by a `AtomMappingScorer`.
+        The `MinimalSpanningTreeNetworkGenerator`, builds an minimal spanning tree (MST) network for a given set of `Component` s.\
+        The `Transformation` s of the Network are represented by an `AtomMapping` s, which are scored by a `AtomMappingScorer`.


As above re: Transformation references.

IAlibay · 2024-11-20T12:50:40Z

src/konnektor/network_planners/generators/explicit_network_generator.py

@@ -25,11 +25,11 @@ def __init__(
        Parameters
        ----------
        mapper: AtomMapper
-            the atom mapper is required, to define the connection between two ligands.
+            Defines the connection between two ligands.


Capitalization isn't consistent - here you capitalize, other places you dont.

Personally I prefer it if these are capitalized, that matches the NumPy docstyle we aim to use everywhere. I'm not sure if the non-capitalized approach folllows some other docstring style so maybe I'm missing something.

atravitz linked an issue Oct 25, 2024 that may be closed by this pull request

Review docstrings #85

Open

atravitz marked this pull request as ready for review October 28, 2024 17:57

atravitz requested a review from RiesBen October 28, 2024 18:00

RiesBen reviewed Oct 29, 2024

View reviewed changes

atravitz changed the title ~~Draft: 85 review docstrings~~ 85 review docstrings Nov 8, 2024

atravitz commented Nov 12, 2024

View reviewed changes

hannahbaumann requested changes Nov 13, 2024

View reviewed changes

atravitz and others added 13 commits November 15, 2024 09:12

renaming LigandNetworkPlanner to NetworkPlanner in docs

9cb9fc3

updating max_concatenator docstring to reflect code

af0bf95

fixing some class names in docstrings

037114e

fixing typos

f1dd647

fixing typos in a few generators

5720c00

finishing first pass of generator docstring

9a1fcce

mapper->mappers

fae4273

fixing network typo

3e4a89c

removing completed todo

d126c2c

Apply suggestions from code review

2a3170b

Co-authored-by: Hannah Baumann <43765638+hannahbaumann@users.noreply.github.com>

adding to maximalnetworkgenerator docstring

1df4ab7

adding description for network_generator argument

6e61290

making ClusterNetworkGenerator require mappers param to match parent …

ae0973b

…class

atravitz force-pushed the 85-review-docstrings branch from 93d133a to ae0973b Compare November 15, 2024 17:12

Revert "making ClusterNetworkGenerator require mappers param to match…

6c237ac

… parent class" This reverts commit ae0973b.

atravitz mentioned this pull request Nov 15, 2024

should NetworkPlanners require mappers? #93

Open

fixing typo in file name

b66f0e1

hannahbaumann self-assigned this Nov 20, 2024

IAlibay approved these changes Nov 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

85 review docstrings #86

85 review docstrings #86

atravitz commented Oct 25, 2024

codecov bot commented Oct 25, 2024 •

edited

Loading

RiesBen Oct 29, 2024

atravitz Nov 8, 2024

hannahbaumann Nov 13, 2024

atravitz Nov 15, 2024

RiesBen Oct 29, 2024

atravitz Nov 8, 2024

hannahbaumann Nov 13, 2024

atravitz Nov 15, 2024 •

edited

Loading

hannahbaumann Nov 20, 2024

IAlibay Nov 20, 2024

atravitz left a comment

atravitz Nov 8, 2024

atravitz Nov 8, 2024

hannahbaumann left a comment

hannahbaumann Nov 13, 2024

hannahbaumann Nov 13, 2024

atravitz Nov 15, 2024

IAlibay Nov 20, 2024

hannahbaumann Nov 13, 2024

atravitz Nov 15, 2024

IAlibay left a comment

IAlibay Nov 20, 2024

IAlibay Nov 20, 2024

IAlibay Nov 20, 2024

IAlibay Nov 20, 2024

IAlibay Nov 20, 2024

IAlibay Nov 20, 2024

IAlibay Nov 20, 2024

IAlibay Nov 20, 2024

IAlibay Nov 20, 2024

	_initial_edge_lister=None, ## TODO: should this be _initial_edge_lister?
	_initial_edge_lister=_initial_edge_lister,

85 review docstrings #86

Are you sure you want to change the base?

85 review docstrings #86

Conversation

atravitz commented Oct 25, 2024

codecov bot commented Oct 25, 2024 • edited Loading

Codecov Report

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

atravitz Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atravitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannahbaumann 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 25, 2024 •

edited

Loading

atravitz Nov 15, 2024 •

edited

Loading