diff --git a/arc/molecule/graph.pxd b/arc/molecule/graph.pxd index 8609576e6a..59687f5f79 100644 --- a/arc/molecule/graph.pxd +++ b/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) diff --git a/arc/molecule/graph.pyx b/arc/molecule/graph.pyx index d6382048c9..d6a8dea3c8 100644 --- a/arc/molecule/graph.pyx +++ b/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 diff --git a/arc/molecule/graph_test.py b/arc/molecule/graph_test.py index 6a8f11b2c1..4f9f2dfabd 100644 --- a/arc/molecule/graph_test.py +++ b/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. diff --git a/arc/molecule/symmetry_test.py b/arc/molecule/symmetry_test.py index cc32f28881..1d5e846daa 100644 --- a/arc/molecule/symmetry_test.py +++ b/arc/molecule/symmetry_test.py @@ -3,6 +3,7 @@ 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, @@ -10,6 +11,20 @@ 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))