Make the molecular graph's edge and cycle order independent of the hash seed

arc/molecule/graph.pxd

@@ -54,6 +54,8 @@ cdef class Graph(object):
 
     cpdef list get_all_edges(self)
 
+    cpdef list order_vertex_set(self, set vertex_set)
+
     cpdef dict get_edges(self, Vertex vertex)
 
     cpdef Edge get_edge(self, Vertex vertex1, Vertex vertex2)

arc/molecule/graph.pyx

@@ -218,18 +218,53 @@ cdef class Graph(object):
 
     cpdef list get_all_edges(self):
         """
-        Returns a list of all edges in the graph.
+        Returns a list of all edges in the graph, each edge appearing once,
+        ordered by the graph's vertex order and, within a vertex, by the order
+        in which its edges were added. The order does not depend on the hash
+        values of the edges, so it is identical in every process.
         """
-        cdef set edge_set
+        cdef list edges
+        cdef set seen
         cdef Vertex vertex
         cdef Edge edge
 
-        edge_set = set()
+        edges = []
+        seen = set()
         for vertex in self.vertices:
             for edge in vertex.edges.values():
-                edge_set.add(edge)
+                if id(edge) not in seen:
+                    seen.add(id(edge))
+                    edges.append(edge)
+
+        return edges
+
+    cpdef list order_vertex_set(self, set vertex_set):
+        """
+        Returns the vertices of `vertex_set` as a list, ordered by their position
+        in the graph's vertex list. The order does not depend on the hash values
+        of the vertices, so it is identical in every process.
+
+        Every vertex of `vertex_set` must be a vertex of this graph; a vertex that
+        is not raises a ValueError.
+        """
+        cdef list ordered
+        cdef set identities, found
+        cdef Vertex vertex
 
-        return list(edge_set)
+        identities = {id(vertex) for vertex in vertex_set}
+
+        ordered = []
+        found = set()
+        for vertex in self.vertices:
+            if id(vertex) in identities and id(vertex) not in found:
+                found.add(id(vertex))
+                ordered.append(vertex)
+
+        if len(found) < len(identities):
+            raise ValueError(f'Attempted to order {len(identities)} vertices of which '
+                             f'{len(identities) - len(found)} are not in the graph.')
+
+        return ordered
 
     cpdef dict get_edges(self, Vertex vertex):
         """
@@ -615,9 +650,12 @@ cdef class Graph(object):
     cpdef list get_polycycles(self):
         """
         Return a list of cycles that are polycyclic.
-        In other words, merge the cycles which are fused or spirocyclic into 
-        a single polycyclic cycle, and return only those cycles. 
+        In other words, merge the cycles which are fused or spirocyclic into
+        a single polycyclic cycle, and return only those cycles.
         Cycles which are not polycyclic are not returned.
+
+        The vertices of each returned cycle are ordered by their position in the
+        graph's vertex list, so the order is identical in every process.
         """
         cdef list polycyclic_vertices, continuous_cycles, sssr
         cdef set polycyclic_cycle
@@ -652,7 +690,7 @@ cdef class Graph(object):
                         polycyclic_cycle.update(cycle)
 
             # convert each set to a list
-            continuous_cycles = [list(cycle) for cycle in continuous_cycles]
+            continuous_cycles = [self.order_vertex_set(cycle) for cycle in continuous_cycles]
             return continuous_cycles
 
     cpdef list get_monocycles(self):
@@ -690,7 +728,10 @@ cdef class Graph(object):
         """
         Get all disjoint monocyclic and polycyclic cycle clusters in the molecule.
         Takes the RC and recursively merges all cycles which share vertices.
-
+
+        The vertices of each returned cycle are ordered by their position in the
+        graph's vertex list, so the order is identical in every process.
+
         Returns: monocyclic_cycles, polycyclic_cycles
         """
         cdef list rc, cycle_list, cycle_sets, monocyclic_cycles, polycyclic_cycles
@@ -708,8 +749,8 @@ cdef class Graph(object):
         monocyclic_cycles, polycyclic_cycles = self._merge_cycles(cycle_sets)
 
         # Convert cycles back to lists
-        monocyclic_cycles = [list(cycle_set) for cycle_set in monocyclic_cycles]
-        polycyclic_cycles = [list(cycle_set) for cycle_set in polycyclic_cycles]
+        monocyclic_cycles = [self.order_vertex_set(cycle_set) for cycle_set in monocyclic_cycles]
+        polycyclic_cycles = [self.order_vertex_set(cycle_set) for cycle_set in polycyclic_cycles]
 
         return monocyclic_cycles, polycyclic_cycles
 
@@ -779,7 +820,10 @@ cdef class Graph(object):
     cpdef list get_all_cycles_of_size(self, int size):
         """
         Return a list of the all non-duplicate rings with length 'size'. The
-        algorithm implements was adapted from a description by Fan, Panaye,
+        vertices of each ring are ordered by their position in the graph's vertex
+        list, so the order is identical in every process.
+
+        The algorithm implements was adapted from a description by Fan, Panaye,
         Doucet, and Barbu (doi: 10.1021/ci00015a002)
 
         B. T. Fan, A. Panaye, J. P. Doucet, and A. Barbu. "Ring Perception: A
@@ -884,7 +928,7 @@ cdef class Graph(object):
                 cycle_set_list.append(set1)
 
         #transform back to list of lists:
-        cycle_set_list = [list(set1) for set1 in cycle_set_list]
+        cycle_set_list = [self.order_vertex_set(set1) for set1 in cycle_set_list]
 
         return cycle_set_list
 

arc/molecule/graph_test.py

@@ -1,11 +1,38 @@
 #!/usr/bin/env python3
 # encoding: utf-8
 
+import os
+import subprocess
+import sys
 import unittest
 
 from arc.molecule.graph import Edge, Graph, Vertex
 
 
+REPOSITORY_DIRECTORY = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+
+
+def outputs_at_hash_seeds(script, seeds):
+    """
+    Run `script` in a subprocess once per hash seed in `seeds`, and return the stripped standard
+    output of each run. The subprocesses are given the repository as their working directory and at
+    the front of PYTHONPATH, so they import the tree under test rather than an installed ARC while
+    keeping whatever else the environment already put on the path.
+
+    Raises a RuntimeError if any of the subprocesses exits with a non-zero return code.
+    """
+    python_path = os.pathsep.join(path for path in (REPOSITORY_DIRECTORY, os.environ.get('PYTHONPATH', '')) if path)
+    outputs = list()
+    for seed in seeds:
+        environment = dict(os.environ, PYTHONHASHSEED=seed, PYTHONPATH=python_path)
+        result = subprocess.run([sys.executable, '-c', script], capture_output=True, text=True,
+                                env=environment, cwd=REPOSITORY_DIRECTORY, timeout=600)
+        if result.returncode:
+            raise RuntimeError(f'The subprocess run with PYTHONHASHSEED={seed} failed:\n{result.stderr}')
+        outputs.append(result.stdout.strip())
+    return outputs
+
+
 class TestGraph(unittest.TestCase):
     """
     Contains unit tests of the Vertex, Edge, and Graph classes. Most of the
@@ -108,6 +135,86 @@ def test_get_all_edges(self):
         self.assertIsInstance(edges, list)
         self.assertEqual(len(edges), 5)
 
+    def test_get_all_edges_orders_the_edges_by_vertex(self):
+        """
+        Test that Graph.get_all_edges() returns the edges in vertex order, each edge once.
+        """
+        expected = []
+        for vertex in self.graph.vertices:
+            for edge in vertex.edges.values():
+                if not any(edge is seen for seen in expected):
+                    expected.append(edge)
+        self.assertEqual(self.graph.get_all_edges(), expected)
+
+    def test_get_all_edges_order_does_not_depend_on_the_hash_seed(self):
+        """
+        Test that Graph.get_all_edges() returns the same order in processes with different hash seeds.
+        """
+        script = ('from arc.molecule.molecule import Molecule\n'
+                  'mol = Molecule(smiles="c1ccccc1Cc1ccccc1")\n'
+                  'atoms = mol.atoms\n'
+                  'print([(atoms.index(edge.vertex1), atoms.index(edge.vertex2)) '
+                  'for edge in mol.get_all_edges()])\n')
+        outputs = outputs_at_hash_seeds(script, ('1', '35'))
+        self.assertTrue(outputs[0])
+        self.assertEqual(len(set(outputs)), 1, f'The edge order differs between hash seeds: {outputs}')
+
+    def test_order_vertex_set(self):
+        """
+        Test that Graph.order_vertex_set() returns the vertices in the graph's vertex order.
+        """
+        vertices = self.graph.vertices
+        self.assertEqual(self.graph.order_vertex_set({vertices[4], vertices[1], vertices[3]}),
+                         [vertices[1], vertices[3], vertices[4]])
+        self.assertEqual(self.graph.order_vertex_set(set()), [])
+        self.assertEqual(self.graph.order_vertex_set(set(vertices)), vertices)
+
+    def test_order_vertex_set_rejects_a_vertex_that_is_not_in_the_graph(self):
+        """
+        Test that Graph.order_vertex_set() raises a ValueError instead of dropping a foreign vertex.
+        """
+        vertices = self.graph.vertices
+        with self.assertRaises(ValueError):
+            self.graph.order_vertex_set({Vertex()})
+        with self.assertRaises(ValueError):
+            self.graph.order_vertex_set({vertices[0], vertices[2], Vertex()})
+        copied = self.graph.copy(deep=True)
+        with self.assertRaises(ValueError):
+            self.graph.order_vertex_set(set(copied.vertices))
+
+    def test_order_vertex_set_counts_a_repeated_vertex_once(self):
+        """
+        Test that Graph.order_vertex_set() returns a vertex once and still rejects a foreign vertex
+        when the graph's vertex list holds the same vertex twice.
+        """
+        vertices = self.graph.vertices
+        repeated = Graph(vertices=[vertices[0], vertices[0], vertices[1]])
+        self.assertEqual(repeated.order_vertex_set({vertices[0], vertices[1]}),
+                         [vertices[0], vertices[1]])
+        with self.assertRaises(ValueError):
+            repeated.order_vertex_set({vertices[0], Vertex()})
+
+    def test_cycle_order_does_not_depend_on_the_hash_seed(self):
+        """
+        Test that the cycles a graph returns are ordered identically in processes with different hash seeds.
+
+        The comparison covers the order of the vertices within one cycle and the order of the cycles
+        within the returned list.
+        """
+        script = ('from arc.molecule.molecule import Molecule\n'
+                  'for smiles in ("C1CC2CCC1C2", "c1ccccc1Cc1ccccc1", "C1CC2CCC3CCC1C23"):\n'
+                  '    mol = Molecule(smiles=smiles)\n'
+                  '    atoms = mol.atoms\n'
+                  '    index = lambda cycle: [atoms.index(atom) for atom in cycle]\n'
+                  '    monocyclic, polycyclic = mol.get_disparate_cycles()\n'
+                  '    print(smiles, [index(cycle) for cycle in monocyclic + polycyclic])\n'
+                  '    print(smiles, [index(cycle) for cycle in mol.get_polycycles()])\n'
+                  '    print(smiles, [index(cycle) for cycle in mol.get_all_cycles_of_size(5)])\n'
+                  '    print(smiles, [index(cycle) for cycle in mol.get_smallest_set_of_smallest_rings()])\n')
+        outputs = outputs_at_hash_seeds(script, ('1', '5', '87'))
+        self.assertTrue(outputs[0])
+        self.assertEqual(len(set(outputs)), 1, f'The cycle order differs between hash seeds: {outputs}')
+
     def test_has_vertex(self):
         """
         Test the Graph.has_vertex() method.

arc/molecule/symmetry_test.py

@@ -3,13 +3,28 @@
 
 import unittest
 
+from arc.molecule.graph_test import outputs_at_hash_seeds
 from arc.molecule.molecule import Molecule
 from arc.molecule.resonance import generate_optimal_aromatic_resonance_structures
 from arc.molecule.symmetry import (calculate_atom_symmetry_number, calculate_axis_symmetry_number,
     calculate_bond_symmetry_number, calculate_cyclic_symmetry_number, _indistinguishable)
 from arc.species.species import ARCSpecies
 
 
+HASH_SEED_SPECIES = ('C1CC2CCC1C2',
+                     'C1CC2CCC1CC2',
+                     'C1CC2CCC3CCC1C23',
+                     'C1CC2CCC1O2',
+                     'C1=CC2C=CC1C2',
+                     'c1cc2ccc3cccc4ccc(c1)c2c34',
+                     'c1ccc2c(c1)-c1cccc3cccc2c13',
+                     'c1cc2cccc3c4cccc5cccc(c(c1)c23)c54',
+                     'c1cc2ccc3ccc4ccc5ccc6ccc1c1c2c3c4c5c61',
+                     '[CH2]c1ccc2ccccc2c1')
+
+HASH_SEEDS = ('1', '3', '5', '13')
+
+
 class TestMoleculeSymmetry(unittest.TestCase):
     """
     Contains unit tests of the methods for computing symmetry numbers for a
@@ -676,6 +691,39 @@ def test_indistinguishable_2(self):
         # O is different from H
         self.assertFalse(_indistinguishable(mol.atoms[6], mol.atoms[7]))
 
+    def test_symmetry_number_does_not_depend_on_the_hash_seed(self):
+        """
+        Test that calculate_symmetry_number() returns the same value in processes with different hash seeds.
+
+        The bridged polycyclics and fused aromatics of HASH_SEED_SPECIES reach
+        calculate_cyclic_symmetry_number() through get_disparate_cycles(). Only agreement between the
+        processes is asserted, not the value they agree on.
+        """
+        script = ('from arc.molecule.molecule import Molecule\n'
+                  'from arc.molecule.symmetry import calculate_symmetry_number\n'
+                  f'print([calculate_symmetry_number(Molecule(smiles=smiles)) for smiles in {HASH_SEED_SPECIES}])\n')
+        symmetry_numbers = outputs_at_hash_seeds(script, HASH_SEEDS)
+        self.assertTrue(symmetry_numbers[0])
+        self.assertEqual(len(set(symmetry_numbers)), 1,
+                         f'The symmetry numbers differ between hash seeds: {symmetry_numbers}')
+
+    def test_species_symmetry_number_does_not_depend_on_the_hash_seed(self):
+        """
+        Test that ARCSpecies.get_symmetry_number() returns the same value in processes with different hash seeds.
+
+        This is the entry point production uses. It reaches the symmetry code through
+        get_resonance_hybrid() rather than through the molecule it was given, so the resonance layer
+        lies between the caller and the cycles. Only agreement between the processes is asserted, not
+        the value they agree on.
+        """
+        script = ('from arc.species.species import ARCSpecies\n'
+                  'print([ARCSpecies(label=f"species{index}", smiles=smiles).get_symmetry_number() '
+                  f'for index, smiles in enumerate({HASH_SEED_SPECIES})])\n')
+        symmetry_numbers = outputs_at_hash_seeds(script, HASH_SEEDS)
+        self.assertTrue(symmetry_numbers[0])
+        self.assertEqual(len(set(symmetry_numbers)), 1,
+                         f'The symmetry numbers differ between hash seeds: {symmetry_numbers}')
+
 
 if __name__ == '__main__':
     unittest.main(testRunner=unittest.TextTestRunner(verbosity=2))