Frm rustworkx - cleanup...

gerrychain/accept.py

@@ -21,7 +21,7 @@ def cut_edge_accept(partition: Partition) -> bool:
     Always accepts the flip if the number of cut_edges increases.
     Otherwise, uses the Metropolis criterion to decide.
 
-    frm: TODO:  Add documentation on what the "Metropolis criterion" is...
+    frm: TODO: Documentation: Add documentation on what the "Metropolis criterion" is...
 
     :param partition: The current partition to accept a flip from.
     :type partition: Partition

gerrychain/constraints/contiguity.py

@@ -8,21 +8,28 @@
 
 from ..graph import Graph
 
-# frm: TODO:  Think about the efficiency of the routines in this module.  Almost all
+# frm: TODO: Performance: Think about the efficiency of the routines in this module.  Almost all
 #               of these involve traversing the entire graph, and I fear that callers
 #               might make multiple calls.
 #
 #               Possible solutions are to 1) speed up these routines somehow and 2) cache
 #               results so that at least we don't do the traversals over and over.
 
-# frm: TODO: Rethink WTF this module is all about.  
+# frm: TODO: Refactoring: Rethink WTF this module is all about.  
 # 
 # It seems like a grab bag for lots of different things - used in different places.
 # 
 # What got me to write this comment was looking at the signature for def contiguous() 
 # which operates on a partition, but lots of other routines here operate on graphs or
 # other things.  So, what is going on?
 #
+# Peter replied to this comment in a pull request:
+#
+#     So anything that is prefixed with an underscore in here should be a helper 
+#     function and not a part of the public API. It looks like, other than 
+#     is_connected_bfs (which should probably be marked "private" with an 
+#     underscore) everything here is acting like an updater.
+#
 
 
 def _are_reachable(graph: Graph, start_node: Any, avoid: Callable, targets: Any) -> bool:
@@ -41,7 +48,7 @@ def _are_reachable(graph: Graph, start_node: Any, avoid: Callable, targets: Any)
         It should take in three parameters: the start node, the end node, and
         the edges to avoid. It should return True if the edge should be avoided,
         False otherwise.
-        # frm: TODO:  Fix the comment above about the "avoid" function parameter.
+        # frm: TODO: Documentation:  Fix the comment above about the "avoid" function parameter.
         #               It may have once been accurate, but the original code below
         #               passed parameters to it of (node_id, neighbor_node_id, edge_data_dict)
         #               from NetworkX.Graph._succ  So, "the edges to avoid" above is wrong.
@@ -112,7 +119,7 @@ def _are_reachable(graph: Graph, start_node: Any, avoid: Callable, targets: Any)
                 seen[neighbor] = neighbor_distance
                 push(fringe, (neighbor_distance, next(c), neighbor))
 
-    # frm: TODO:  Simplify this code.  It computes distances and counts but
+    # frm: TODO: Refactoring:  Simplify this code.  It computes distances and counts but
     #               never uses them.  These must be relics of code copied 
     #               from somewhere else where it had more uses...
 
@@ -233,11 +240,6 @@ def contiguous(partition: Partition) -> bool:
     :returns: Whether the partition is contiguous
     :rtype: bool
     """
-    # frm: Original code:
-    #
-    # return all(
-    #     nx.is_connected(partition.subgraphs[part]) for part in _affected_parts(partition)
-    # )
 
     return all(
         is_connected_bfs(partition.subgraphs[part]) for part in _affected_parts(partition)
@@ -255,16 +257,21 @@ def contiguous_bfs(partition: Partition) -> bool:
     :rtype: bool
     """
 
-    # frm: TODO:   Try to figure out why this routine exists.  It seems to be
-    #               exactly the same conceptually as contiguous().  It looks 
-    #               at the "affected" parts - those that have changed node
-    #               assignments from parent, and sees if those parts are 
-    #               contiguous.
+    # frm: TODO: Refactoring:  Figure out why this routine, contiguous_bfs() exists.  
     #
-    #               For now, I have just replaced the existing code which depended
-    #               on NX with a call on contiguous(partition).
+    # It is mentioned in __init__.py so maybe it is used externally in legacy code.
+    #
+    # However, I have changed the code so that it just calls contiguous() and all
+    # of the tests pass, so I am going to assume that my comment below is accurate, 
+    # that is, I am assuming that this function does not need to exist independently
+    # except for legacy purposes.  Stated differently, if someone can verify that 
+    # this routine is NOT needed for legacy purposes, then we can just delete it.
+    # 
+    # It seems to be exactly the same conceptually as contiguous().  It looks 
+    # at the "affected" parts - those that have changed node
+    # assignments from parent, and sees if those parts are 
+    # contiguous.
     #
-
     # frm: Original Code: 
     #
     #    parts_to_check = _affected_parts(partition)
@@ -288,10 +295,6 @@ def number_of_contiguous_parts(partition: Partition) -> int:
     :returns: Number of contiguous parts in the partition.
     :rtype: int
     """
-    # frm: Original Code:
-    # parts = partition.assignment.parts
-    # return sum(1 for part in parts if nx.is_connected(partition.subgraphs[part]))
-    #
     parts = partition.assignment.parts
     return sum(1 for part in parts if is_connected_bfs(partition.subgraphs[part]))
 
@@ -316,7 +319,7 @@ def contiguous_components(partition: Partition) -> Dict[int, list]:
     :rtype: dict
     """
 
-    # frm: TODO:  NX vs RX Issues here:
+    # frm: TODO: Documentation: Migration Guide:  NX vs RX Issues here:
     #
     # The call on subgraph() below is perhaps problematic because it will renumber
     # node_ids...
@@ -333,12 +336,6 @@ def contiguous_components(partition: Partition) -> Dict[int, list]:
     #    3) From each part's subgraph to the subgraphs of contiguous_components...
     #
 
-    # frm: Original Code:
-    #    return {
-    #        part: [subgraph.subgraph(nodes) for nodes in nx.connected_components(subgraph)]
-    #        for part, subgraph in partition.subgraphs.items()
-    #    }
-    #
     connected_components_in_each_partition = {}
     for part, subgraph in partition.subgraphs.items():
         # create a subgraph for each set of connected nodes in the part's nodes
@@ -379,11 +376,11 @@ def _bfs(graph: Dict[int, list]) -> bool:
 
     return num_nodes == len(visited)
 
-# frm: TODO:  Verify that is_connected_bfs() works - add a test or two...
+# frm: TODO: Testing:  Verify that is_connected_bfs() works - add a test or two...
 
-# frm: TODO:  Move this code into graph.py.  It is all about the Graph...
+# frm: TODO: Refactoring:  Move this code into graph.py.  It is all about the Graph...
 
-# frm: Code obtained from the web - probably could be optimized...
+# frm: TODO: Documentation: This code was obtained from the web - probably could be optimized...
 #       This code replaced calls on nx.is_connected()
 def is_connected_bfs(graph: Graph):
     if not graph: