From 8b0977d14753dece4191fd0d6cf22f03fe6ffc72 Mon Sep 17 00:00:00 2001 From: Calvin Pieters Date: Mon, 17 Aug 2026 13:18:12 +0300 Subject: [PATCH 1/2] Order the edges and cycles a graph returns by vertex instead of by hash Graph.get_all_edges() de-duplicated the edges through a set of Edge objects and returned list(edge_set), and get_disparate_cycles(), get_polycycles() and get_all_cycles_of_size() each built their cycles as sets of vertices and handed them back as list(cycle_set). In both cases the order came out of the set's iteration. Atom and Bond hash on their symbols and bond order rather than on identity, and those hashes are derived from string hashes, which Python randomises per process. Every carbon in a molecule therefore lands in one hash bucket, and both the edge order and the vertex order within a cycle differ from one process to the next. Molecule perception consumes the edge order: generate_lewis_structure() walks the bond list in an A* search whose equal-cost states are explored in the order the bonds are listed, so a molecule with several equal-cost Lewis structures could be perceived differently in different processes. Diphenylprolinol methyl ether was perceived with a methoxy C=O double bond and a carbene ring carbon in about 2% of hash seeds, which then failed RDKit's valence check. calculate_cyclic_symmetry_number() consumes the cycle order. For a polycyclic cluster it calls get_largest_ring(ring[0]), seeding a largest-ring search from whichever atom the set happened to list first. In a bridged polycycle the largest ring through the bridge atom is smaller than the largest ring through any other atom, so the ring handed to the symmetry search changes size with the hash seed and the symmetry number changes with it: bicyclo[2.2.1]heptane returned 4 for 163 of the hash seeds 0-200 and 2 for the other 38, and 7-oxabicyclo[2.2.1]heptane returned 4 for 128 and 2 for 73. A symmetry number enters the entropy as -R ln(sigma), so a factor of two is 1.377 cal/mol/K in S and a factor of two in every rate and equilibrium constant for the species. kekulize() consumes the same order through get_all_cycles_of_size(6), which seeds its ring-by-ring resolution with the ring list. The two are one defect and one fix: the cycles are derived from the edges, so ordering the cycles alone leaves get_disparate_cycles() and get_polycycles() hash-seed-dependent through the edge list they are built from. Return the edges in the graph's vertex order, de-duplicating on the edge identities, and return the vertices of every cycle in the graph's vertex order through the new Graph.order_vertex_set(), so both orders are the same in every process. order_vertex_set() matches on vertex identity, which keeps it linear in the number of vertices, and raises rather than dropping a vertex that does not belong to the graph. Both replacements also drop a quadratic term. The old set of Edge objects and the `vertex in vertex_set` membership test both hash on content while comparing by identity, so every bond of one order and every atom of one element shared a single hash bucket. Matching on identity instead takes get_all_edges() on a carbon macrocycle from 0.589 ms to 0.033 ms at 302 atoms and from 46.5 ms to 0.338 ms at 3002 atoms, and the vertex ordering from 13.5 ms to 0.212 ms at 2402 atoms. --- arc/molecule/graph.pxd | 2 ++ arc/molecule/graph.pyx | 70 ++++++++++++++++++++++++++++++++++-------- 2 files changed, 59 insertions(+), 13 deletions(-) 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 From 9fbc87b97d1d76644d4611f2d3bd8b1151c3c29b Mon Sep 17 00:00:00 2001 From: Calvin Pieters Date: Mon, 17 Aug 2026 13:18:12 +0300 Subject: [PATCH 2/2] Test that a graph's edge and cycle order is stable across processes The edge order returned by get_all_edges() and the vertex order within the cycles returned by get_disparate_cycles(), get_polycycles() and get_all_cycles_of_size() used to follow the iteration order of a set of Edge or Vertex objects, which is governed by the per-process randomized string hash, so this asserts the property directly: subprocesses started at different PYTHONHASHSEED values must report the same order, and the same symmetry numbers. The symmetry numbers are checked at both entry points. calculate_symmetry_number() is the one that reads the cycles, and ARCSpecies.get_symmetry_number() is the one production calls, which reaches the symmetry code through get_resonance_hybrid() rather than through the molecule it was given, so the resonance layer is covered too. Both assert only that the processes agree, not what they agree on. What calculate_cyclic_symmetry_number() should return for a bridged polycycle or a peri-fused aromatic is a separate question from whether it returns the same thing twice, and asserting a value here would fix the wrong one in place. order_vertex_set() is covered for the ordering itself and for its rejection of a vertex that does not belong to the graph, including the vertices of a copy of the graph, which compare unequal to the originals and would otherwise be dropped. The child processes are given PYTHONPATH and a working directory explicitly. A subprocess inherits the parent's working directory but not pytest's sys.path, so without it the child imports whichever ARC `import arc` resolves to -- which, with an editable install present, is not necessarily the tree under test. The test then either fails spuriously when run from outside the repository root, or passes while having validated a different checkout. --- arc/molecule/graph_test.py | 107 ++++++++++++++++++++++++++++++++++ arc/molecule/symmetry_test.py | 48 +++++++++++++++ 2 files changed, 155 insertions(+) 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))