From 5ec5b0ff20670456dcfade13407dc7a2ce16d85e Mon Sep 17 00:00:00 2001 From: Calvin Pieters Date: Sun, 16 Aug 2026 23:16:00 +0300 Subject: [PATCH 1/2] Report an RDKit sanitization failure instead of swallowing it ``to_rdkit_mol()`` caught ``AtomValenceException`` from ``Chem.SanitizeMol()`` with a bare ``pass``. It then returned an unsanitized RDMol, which travels on to its callers, and the same valence problem re-emerges much later inside ``EmbedMultipleConfs`` where it is attributed to embedding. The evidence for the original failure was destroyed at the point it was cheapest to read. Keep the swallow: all three callers of ``to_rdkit_mol()`` (``rdkit_conf_from_mol``, ``conformers.embed_rdkit`` and the goflow_ts TS adapter) use the returned molecule, and returning ``None`` here would be a behaviour change with a blast radius well beyond this fix. Only make the failure visible. The log level was chosen from a measurement, not from the comment the code carried. Instrumenting the except and running ``arc/species/``, ``arc/molecule/`` and ``arc/reaction/`` (936 tests) produced zero firings. Running ``arc/job/adapters/ts/linear_test.py`` produced two, and neither is the ``[C-]#[O+]`` case the removed comment named: both are singlet biradicals (multiplicity 1, no formal charges, two lone-pair carbons) reached through ``determine_chirality`` -> ``embed_rdkit`` while computing a reaction atom map. Carbon monoxide never reaches this code in a normal species flow, since ARC short-circuits conformer generation for diatomics. The exception is therefore rare, is not dominated by a known-benign case, and every occurrence means an unsanitized molecule is being handed onward - so a plain ``logger.warning`` is both affordable and warranted. The message names the exception class and its text so the next occurrence self-identifies rather than needing this investigation repeated. --- arc/species/converter.py | 8 +++++--- arc/species/converter_test.py | 11 +++++++++++ 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/arc/species/converter.py b/arc/species/converter.py index 5075ae2279..f14fefed45 100644 --- a/arc/species/converter.py +++ b/arc/species/converter.py @@ -1614,6 +1614,7 @@ def to_rdkit_mol(mol, remove_h=False, sanitize=True): Returns: RDMol: An RDKit molecule object corresponding to the input RMG Molecule object. + If sanitization was requested but failed, an unsanitized molecule is returned and a warning is logged. """ atom_id_map = dict() @@ -1654,9 +1655,10 @@ def to_rdkit_mol(mol, remove_h=False, sanitize=True): if sanitize: try: Chem.SanitizeMol(rd_mol) - except AtomValenceException: - # [C-]#[O+] raises this - pass + except AtomValenceException as e: + logger.warning(f'Could not sanitize the RDKit molecule of {mol_copy.get_formula()} ' + f'(multiplicity {mol_copy.multiplicity}), returning an unsanitized RDKit molecule. ' + f'Got {e.__class__.__name__}: {e}') if remove_h: rd_mol = Chem.RemoveHs(rd_mol, sanitize=sanitize) return rd_mol diff --git a/arc/species/converter_test.py b/arc/species/converter_test.py index 033bdf78ac..0dc7d6224f 100644 --- a/arc/species/converter_test.py +++ b/arc/species/converter_test.py @@ -3432,6 +3432,17 @@ def test_to_rdkit_mol(self): rd_mol_block = Chem.MolToMolBlock(rd_mol).splitlines() self._check_atom_connectivity_in_rd_mol_block(spc3.mol, rd_mol_block) + def test_to_rdkit_mol_reports_a_sanitization_failure(self): + """Test that a molecule RDKit refuses to sanitize is reported and still returned""" + spc = ARCSpecies(label='CO', smiles='[C-]#[O+]') + with self.assertLogs('arc', level='WARNING') as captured: + rd_mol = converter.to_rdkit_mol(spc.mol) + self.assertIsInstance(rd_mol, rdchem.Mol) + log = '\n'.join(captured.output) + self.assertIn('AtomValenceException', log) + self.assertIn('Explicit valence', log) + self.assertIn('unsanitized', log) + def test_xyz_to_ase(self): """Test the xyz_to_ase function""" atoms_1 = converter.xyz_to_ase(self.xyz1['dict']) From 013ba1ea797528aa4b461298de0f9532fccb6d5d Mon Sep 17 00:00:00 2001 From: Calvin Pieters Date: Sun, 16 Aug 2026 23:17:06 +0300 Subject: [PATCH 2/2] Stop embed_rdkit from returning a conformer-less molecule ``EmbedMultipleConfs`` does not only raise on failure - for some strained species it returns normally having embedded zero conformers. ``embed_rdkit`` only guarded the raising path, so in that case it returned an RDMol with no conformers and logged nothing at all. Every consumer of that object then reads an empty list, and the first caller to index it gets ``IndexError: list index out of range`` with no record of where the molecule came from. Reproduced on ``C1#CC1``, ``C1#CCC1`` and ``C1#CC#CC#C1``. Treat zero conformers the same as an exception: log a warning naming the species and return ``None``, which is what the ``RDMol | None`` return annotation already promised and what the exception path already did. All four callers already handle ``None`` and none of them is made worse by receiving it: ``get_force_field_energies`` guards with ``if rd_mol is not None``, ``get_force_field_energies_of_conformers`` returns early on ``None``, ``determine_chirality`` skips on ``rd_mol is None or not rd_mol.GetNumConformers()``, and ``species.get_cheap_conformer`` passes it to ``rdkit_force_field``, which returns empty lists for ``None``. A conformer-less molecule and ``None`` therefore produce the same downstream result, minus the crash and plus a log line. --- arc/species/conformers.py | 7 ++++++- arc/species/conformers_test.py | 11 +++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/arc/species/conformers.py b/arc/species/conformers.py index a6d014df53..c03f4bd066 100644 --- a/arc/species/conformers.py +++ b/arc/species/conformers.py @@ -1465,7 +1465,8 @@ def embed_rdkit(label, mol, num_confs=None, xyz=None): xyz (dict, optional): The 3D coordinates. Returns: - RDMol | None: An RDKIt molecule with embedded conformers. + RDMol | None: An RDKIt molecule with embedded conformers, + or ``None`` if no conformers could be embedded. """ if num_confs is None and xyz is None: raise ConformerError(f'Either num_confs or xyz must be set when calling embed_rdkit() for {label}') @@ -1482,6 +1483,10 @@ def embed_rdkit(label, mol, num_confs=None, xyz=None): except Exception as e: logger.warning(f'Could not embed conformers using RDKit for {label}, failed with: {e}') return None + if not rd_mol.GetNumConformers(): + logger.warning(f'Could not embed conformers using RDKit for {label}, ' + f'RDKit returned no conformers without raising an error.') + return None elif xyz is not None: rd_conf = Chem.Conformer(rd_mol.GetNumAtoms()) for i in range(rd_mol.GetNumAtoms()): diff --git a/arc/species/conformers_test.py b/arc/species/conformers_test.py index 9821208bde..2abfa7a161 100644 --- a/arc/species/conformers_test.py +++ b/arc/species/conformers_test.py @@ -681,6 +681,17 @@ def test_embed_rdkit_reports_why_embedding_failed(self): self.assertIsNone(rd_mol) self.assertIn('Bad Conformer Id', '\n'.join(captured.output)) + def test_embed_rdkit_does_not_return_a_conformer_less_molecule(self): + """Test that an embedding which yields no conformers returns None rather than an unusable molecule""" + spc = ARCSpecies(label='c-C3H2', smiles='C1#CC1') + with self.assertLogs('arc', level='WARNING') as captured: + rd_mol = conformers.embed_rdkit(label='c-C3H2', mol=spc.mol, num_confs=5) + if rd_mol is not None: + xyzs = conformers.read_rdkit_embedded_conformers(label='c-C3H2', rd_mol=rd_mol) + self.assertIsInstance(xyzs[0], dict) + self.assertIsNone(rd_mol) + self.assertIn('c-C3H2', '\n'.join(captured.output)) + def test_rdkit_force_field_abandons_an_optimization_that_raises(self): """Test that a raising optimization is logged and attempted once per conformer""" rd_mol = conformers.embed_rdkit(label='CJ', mol=self.cj_spc.mol, num_confs=1)