Changes to get all tests to pass - some cleanup as well

gerrychain/constraints/contiguity.py

@@ -8,21 +8,23 @@
 
 from ..graph import Graph
 
-# frm TODO: Remove this comment about NX dependencies (once we are all set with the work)
-#
-#           NX dependencies:
-#               def _are_reachable(G: nx.Graph, ...)
-#               nx.is_connected(partition.subgraphs[part]) for part in _affected_parts(partition)
-#               adj = nx.to_dict_of_lists(partition.subgraphs[part])
-#
-
 # frm: TODO:  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.  
+# 
+# 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?
+#
+
+
 def _are_reachable(graph: Graph, start_node: Any, avoid: Callable, targets: Any) -> bool:
     """
     A modified version of NetworkX's function
@@ -101,8 +103,6 @@ def _are_reachable(graph: Graph, start_node: Any, avoid: Callable, targets: Any)
             continue  # already searched this node.
         node_distances[node_id] = distance
 
-        dbg_neighbors = graph.neighbors(node_id)
-
         for neighbor in graph.neighbors(node_id):
             if avoid(node_id, neighbor):
                 continue
@@ -360,12 +360,11 @@ def _bfs(graph: Dict[int, list]) -> bool:
     """
     q = [next(iter(graph))]
     visited = set()
-    # frm TODO:  Make sure len() is defined on Graph object...
-    total_vertices = len(graph)
+    num_nodes = len(graph)
 
     # Check if the district has a single vertex. If it does, then simply return
     # `True`, as it's trivially connected.
-    if total_vertices <= 1:
+    if num_nodes <= 1:
         return True
 
     # bfs!
@@ -378,10 +377,12 @@ def _bfs(graph: Dict[int, list]) -> bool:
                 visited.add(neighbor)
                 q += [neighbor]
 
-    return total_vertices == len(visited)
+    return num_nodes == len(visited)
 
 # frm: TODO:  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: Code obtained from the web - probably could be optimized...
 #       This code replaced calls on nx.is_connected()
 def is_connected_bfs(graph: Graph):

gerrychain/grid.py

@@ -15,16 +15,14 @@
 
 import math
 import networkx
-# frm TODO:     Decide whether to leave grid.py as-is, at least for now.
-#               While it imports NetworkX, it eventually creates a new
-#               Graph object which is added to a Partition which will
-#               eventually "freeze" and convert the new Graph object to
-#               be based on RX (under the covers).
-#
-#               So, this can be thought of as legacy code that works just
-#               fine.  In the future if we want to go full RX everywhere
-#               we can decide what to do.
+# frm TODO: Clarify what purpose grid.py serves.  
 #
+# It is a convenience module to help users create toy graphs.  It leverages
+# NX to create graphs, but it returns new Graph objects.  So, legacy user
+# code will need to be at least reviewed to make sure that it properly 
+# copes with new Graph objects.
+# 
+
 from gerrychain.partition import Partition
 from gerrychain.graph import Graph
 from gerrychain.updaters import (

gerrychain/metagraph.py

@@ -27,6 +27,11 @@ def all_cut_edge_flips(partition: Partition) -> Iterator[Dict]:
     :returns: An iterator that yields dictionaries representing the flipped edges.
     :rtype: Iterator[Dict]
     """
+    # frm: TODO:  Add some documentation so a future readef of this code
+    #             will not be as confused as I was...
+
+    # frm: TODO:  Why is this an iterator instead of just a dict?
+
     # frm: For my own edification...  It took me a while to understand why
     #       this routine made sense at a high level.  It finds all edges
     #       on the boundary of districts - those that are "cut edges" 

gerrychain/metrics/partisan.py

@@ -15,7 +15,7 @@ def mean_median(election_results) -> float:
     """
     Computes the Mean-Median score for the given ElectionResults.
     A positive value indicates an advantage for the first party listed
-    in the Election's parties_to_columns dictionary.
+    in the Election's party_names_to_node_attribute_names dictionary.
 
     :param election_results: An ElectionResults object
     :type election_results: ElectionResults
@@ -33,7 +33,7 @@ def mean_thirdian(election_results) -> float:
     """
     Computes the Mean-Median score for the given ElectionResults.
     A positive value indicates an advantage for the first party listed
-    in the Election's parties_to_columns dictionary.
+    in the Election's party_names_to_node_attribute_names dictionary.
 
     The motivation for this score is that the minority party in many
     states struggles to win even a third of the seats.
@@ -57,7 +57,7 @@ def efficiency_gap(election_results) -> float:
     """
     Computes the efficiency gap for the given ElectionResults.
     A positive value indicates an advantage for the first party listed
-    in the Election's parties_to_columns dictionary.
+    in the Election's party_names_to_node_attribute_names dictionary.
 
     :param election_results: An ElectionResults object
     :type election_results: ElectionResults

gerrychain/partition/partition.py

@@ -159,16 +159,16 @@ def _first_time(self, graph, assignment, updaters, use_default_updaters):
         # a RustworkX graph, and make sure it is also a FrozenGraph.  Both
         # of these are important for performance.
 
-        # frm: TODO:  Do we want to continue to allow users to create a Partition
-        #               directly from an NX graph?  I suppose there is no harm...
+        # Note that we automatically convert NetworkX based graphs to use RustworkX
+        # when we create a Partition object.
         #
-        # The answer is YES - creating and manipulating NX Graphs is easy and users
+        # Creating and manipulating NX Graphs is easy and users
         # are familiar with doing so.  It makes sense to preserve the use case of 
         # creating an NX-Graph and then allowing the code to under-the-covers
         # convert to RX - both for legacy compatibility, but also because NX provides
         # a really nice and easy way to create graphs.
         #
-        # So this TODO should be interpreted as a todo-item to update the documentation
+        # TODO: update the documentation
         # to describe the use case of creating a graph using NX.  That documentation
         # should also describe how to post-process results of a MarkovChain run
         # but I haven't figured that out yet...
@@ -179,8 +179,15 @@ def _first_time(self, graph, assignment, updaters, use_default_updaters):
 
         # if a Graph object, make sure it is based on an embedded RustworkX.PyGraph
         if isinstance(graph, Graph):
-            if (graph.is_nx_graph()):
+            # frm: TODO: Remove this short-term hack to do performance testing
+            test_performance_using_NX_graph = False
+            if (graph.is_nx_graph()) and test_performance_using_NX_graph:
+                self.assignment = get_assignment(assignment, graph)
+                print("Performance-Test: using NetworkX for Partition object")
+
+            elif (graph.is_nx_graph()):
 
+                print("Partition: converting NX to RX")
                 # Get the assignment that would be appropriate for the NX-based graph
                 old_nx_assignment = get_assignment(assignment, graph)
 
@@ -195,14 +202,6 @@ def _first_time(self, graph, assignment, updaters, use_default_updaters):
                 )
                 self.assignment = new_rx_assignment
 
-                # We also have to update the _node_id_to_original_node_id_map to refer to the node_ids
-                # in the NX Graph object.
-                _node_id_to_original_node_id_map = {}
-                for node_id in graph.nodes:
-                    original_node_id = graph.node_data(node_id)["__networkx_node__"]
-                    _node_id_to_original_node_id_map[node_id] = original_node_id
-                graph._node_id_to_original_node_id_map = _node_id_to_original_node_id_map
-
             else:
                 self.assignment = get_assignment(assignment, graph)
 
@@ -271,7 +270,7 @@ def __repr__(self):
     def __len__(self):
         return len(self.parts)
 
-    def flip(self, flips: Dict, use_original_node_ids=False) -> "Partition":
+    def flip(self, flips: Dict, use_original_nx_node_ids=False) -> "Partition":
         """
         Returns the new partition obtained by performing the given `flips`
         on this partition.
@@ -281,7 +280,7 @@ def flip(self, flips: Dict, use_original_node_ids=False) -> "Partition":
         :rtype: Partition
         """
 
-        # frm: TODO: Change comments above to document new optional parameter, use_original_node_ids.
+        # frm: TODO: Change comments above to document new optional parameter, use_original_nx_node_ids.
         #
         # This is a new issue that arises from the fact that node_ids in RX are different from those
         # in the original NX graph.  In the pre-RX code, we did not need to distinguish between
@@ -296,10 +295,10 @@ def flip(self, flips: Dict, use_original_node_ids=False) -> "Partition":
         # Note that original node_ids in flips are typically used in tests
         #
 
-        if use_original_node_ids:
+        if use_original_nx_node_ids:
             new_flips = {}
-            for original_node_id, part in flips.items():
-                internal_node_id = self.graph.internal_node_id_for_original_node_id(original_node_id)
+            for original_nx_node_id, part in flips.items():
+                internal_node_id = self.graph.internal_node_id_for_original_nx_node_id(original_nx_node_id)
                 new_flips[internal_node_id] = part
             flips = new_flips
 
@@ -399,9 +398,10 @@ def plot(self, geometries=None, **kwargs):
         import geopandas
 
         if geometries is None:
-            geometries = self.graph.geometry
-            # frm: TODO: Test that self.graph.geometry is not None - but first need to grok
-            #               where this is set (other than Graph.from_geodataframe())
+            if hasattr(self.graph, "geometry"):
+                geometries = self.graph.geometry
+            else:
+                raise Exception("Partition.plot: graph has no geometry data")
 
         if set(geometries.index) != set(self.graph.nodes):
             raise TypeError(

gerrychain/partition/subgraphs.py

@@ -1,15 +1,6 @@
 from typing import List, Any, Tuple
 from ..graph import Graph
 
-# frm: ???: TODO:   Is this ever actually used by any other code?  If so, where and for what?
-#                   YES - it is used as the type of Partition.subgraphs.  So, I need to find
-#                   all access to partition.subgraphs and see how it is used.  It is also
-#                   used in contiguity.py
-#
-#                   This may be an opportunity to encapsulate knowldege of node_indices vs.
-#                   node_names...
-
-
 class SubgraphView:
     """
     A view for accessing subgraphs of :class:`Graph` objects.

gerrychain/proposals/proposals.py

@@ -111,10 +111,8 @@ def slow_reversible_propose_bi(partition: Partition) -> Partition:
     :rtype: Partition
     """
 
-    # frm: TODO:  Rename x to be edge  *sigh*...
-
-    b_nodes = {x[0] for x in partition["cut_edges"]}.union(
-        {x[1] for x in partition["cut_edges"]}
+    b_nodes = {edge[0] for edge in partition["cut_edges"]}.union(
+        {edge[1] for edge in partition["cut_edges"]}
     )
 
     flip = random.choice(list(b_nodes))

gerrychain/proposals/spectral_proposals.py

@@ -33,16 +33,6 @@ def spectral_cut(
     # the return value's node_ids need to be translated back into the appropriate
     # parent node_ids.
 
-    # frm: TODO:  Subtle issue here - in NX there is no difference between a node ID
-    #               and a node index (what you use to get a node from a list), but 
-    #               in RX there is a difference - which manifests most in subgraphs
-    #               where RX goes ahead and renumbers the nodes in the graph.  To
-    #               make subgraphs work, we remember (in a map) what the node "IDs"
-    #               of the parent graph were.
-    #
-    #               The issue here is what the code wants here.  We are in an RX 
-    #               world at this point - so maybe it doesn't matter, but worth 
-    #               thinking about...
     node_list = list(subgraph.nodes)
     num_nodes = len(node_list)
 
@@ -51,13 +41,6 @@ def spectral_cut(
         for edge_id in subgraph.edge_indices:
             subgraph.edge_data(edge_id)["weight"] = random.random()
 
-    # frm TODO: NX vs. RX Issue:   NYI: normalized_laplacian_matrix() for RX
-    #
-    #           Note that while the standard laplacian is straight forward mathematically
-    #           the normalized laplacian is a good bit more complicated.  However, since 
-    #           NetworkX is open source - perhaps we can get permission to just use their
-    #           code to create RX versions...
-
     # Compute the desired laplacian matrix (convert from sparse to dense)
     if lap_type == "normalized":
         laplacian_matrix = (subgraph.normalized_laplacian_matrix()).todense()

gerrychain/proposals/tree_proposals.py

@@ -200,8 +200,6 @@ def dist_pair_edges(part, a, b):
         # frm: Find all edges that cross from district a into district b
         return set(
             e
-            # frm: TODO: edges vs. edge_ids:  edges are wanted here (tuples)
-            # frm: Original Code:    for e in part.graph.edges
             for e in part.graph.edges
             if (
                 (
@@ -305,8 +303,6 @@ def bounded_balance_edge_fn(*args, **kwargs):
     #               the subgraph's node_ids afterwards.
     #
 
-    # frm: TODO:  Clean up the code below - I munged it for debugging ...
-
     # frm: Original Code:
     #    num_possible_districts, nodes = bipartition_tree_random_reversible(
     #        partition.graph.subgraph(subgraph_nodes),
@@ -322,7 +318,7 @@ def bounded_balance_edge_fn(*args, **kwargs):
     num_possible_districts, nodes = result
 
     remaining_nodes = subgraph_nodes - set(nodes)
-    # frm: Notes to Self:  the ** operator below merges the two dicts into a single dict.
+    # Note:  the ** operator below merges the two dicts into a single dict.
     flips = {
         **{node: parts_to_merge[0] for node in nodes},
         **{node: parts_to_merge[1] for node in remaining_nodes},