CREST Adapter - #807
Conversation
d6a83b4 to
c231a43
Compare
3e88c36 to
7674f5f
Compare
9c25f3d to
51368be
Compare
9be935e to
eab7647
Compare
When a TS guess is abandoned, the IRC endpoint species it spawned (e.g. IRC_TS0_1) are deleted. delete_all_species_jobs removed them from self.output and self.species_dict, but removed them from self.species_list by reassignment (self.species_list = [...]), which rebinds only the scheduler's own attribute. ARC.species is the same list object (the scheduler mutates it in-place when it appends the IRC species), so the deleted species stayed in ARC.species. save_project_info_file then iterated ARC.species and looked each label up in the synced self.output, raising KeyError and aborting the run at the final summary step, after all of the quantum-chemistry work had completed. Fix: delete in-place via slice assignment so removals propagate through the shared list the same way the appends do. This alone resolves the reported failure. As defence in depth, save_project_info_file now also skips species missing from self.output in both of its loops, the one writing the .info file and the one writing the _info.yml file, and logs the skipped labels. The _info.yml file is what T3 reads to learn which species ARC computed, so a species dropped from it silently would be indistinguishable from one that was never requested. Tests: the existing test_switch_ts_cleanup now also asserts the removal through the caller's own species list object; a new end-to-end test wires a real ARC to a real Scheduler the way ARC.execute does and writes the info files after a TS guess switch; and a synthetic test covers the summary-writer guards, which the end-to-end path no longer reaches once the removal is correct. (cherry picked from commit 3eb966e) arcbench conflict resolution: arcbench already carries the slice-assignment removal and both save_project_info_file guards under 8f5e604, an earlier variant of this fix. Kept from this commit: the unreported_labels warning, the two docstrings, and the tests. The inline comments 8f5e604 used to explain the slice assignment and the summary-loop guard are dropped, since this commit puts the same explanation in the docstrings and ARC code carries no inline comments. main_test's synthetic ARC.__new__ / SimpleNamespace version of test_save_project_info_file_skips_deleted_species is replaced by this commit's real-ARC version, which also asserts the _info.yml file, plus the end-to-end test; 'import time' and 'from types import SimpleNamespace' had no other user and are removed.
An atom has nothing to optimize, so ARC never runs an opt job for it and
ARCSpecies.final_xyz is only ever populated as a side effect of someone
calling get_xyz(generate=True), which routes through get_cheap_conformer().
get_xyz() short-circuits on the first geometry it finds:
xyz = self.final_xyz or self.initial_xyz or self.most_stable_conformer \
or conf or self.cheap_conformer
so an atom that was given an explicit geometry never reaches
get_cheap_conformer(), and final_xyz stays None. Two consequences:
1. ARCReaction.check_done_opt_r_n_p() gates TS-search dispatch on every
reactant and product having a non-None final_xyz. An atom supplied with
an xyz therefore pins done_opt_r_n_p to False for the rest of the run,
and no TS-guess job is ever spawned -- silently, with no error.
An atom given only a SMILES is unaffected: Scheduler.__init__ calls
check_atom_balance() (scheduler.py:408) right before
check_done_opt_r_n_p() (:409), and check_atom_balance() calls
get_xyz(generate=True), which populates final_xyz for a bare atom.
2. A standalone atom that participates in no reaction never enters that
loop at all, so run_sp_job() submits get_xyz(generate=False), i.e. None,
as the sp job geometry.
ARCSpecies.__init__ now populates final_xyz for a monoatomic species,
reusing get_xyz(generate=True) so precedence and conformer generation stay
in one place. Guarded by the existing is_monoatomic() predicate.
Adds a reaction-level regression test that replays the full scheduler
sequence (rebind, check_atom_balance, check_done_opt_r_n_p) with an atomic
reactant given an explicit xyz.
(cherry picked from commit 75922ec)
arcbench conflict resolution: arcbench already carries a hand-rolled
_init_monoatomic_geometry (from ca7ce14) which re-derived get_xyz's precedence
chain and synthesized origin coordinates itself. Git applied this commit's
method alongside it rather than over it, leaving two definitions and two call
sites; this commit's version is kept and arcbench's removed. The two are
behaviour-equivalent for the case arcbench's covered - get_xyz(generate=True)
returns origin coordinates for a bare [H]/[O]/[Cl] - and this version also
handles a species whose is_monoatomic() is decided from mol_list or from the
xyz rather than from mol.
reaction_test keeps BOTH tests: arcbench's covers an atomic reactant given only
a SMILES, this commit's covers one given an explicit geometry, which is the
case the fix addresses. species_test takes this commit's four
test_init_monoatomic_geometry_* tests, which subsume arcbench's three.
test_xyz_from_dict's fixtures become diatomic CH here, which is what restores
the test's original conformer-routing intent; arcbench's monoatomic
expectations and the comment describing them are therefore dropped.
ArkaneAdapter carried a second layer of the A+A thermo fix on arcbench: render_arkane_input_template() collapsed every pair of species that its _dedup_thermo_species_list() judged identical into a single Arkane thermo block, recorded the dropped label in _thermo_duplicate_aliases, and _propagate_duplicate_species_thermo() then copied the representative's parsed thermo, E0, symmetry and optical isomers onto the dropped species. The test it applied is graph isomorphism plus equal multiplicity. ARC's Molecule is a 2D connectivity graph with no stereochemistry and no geometry, so that test is true of species whose thermochemistry differs: cis/trans-2-butene, cis/trans-1,2-dimethylcyclohexane, R/S enantiomers, and two conformers of one SMILES submitted as separate species. In each of those cases one species' computed thermodynamics was assigned to the other, with the direction decided by the order of the species list. It was not scoped to A+A reactions. arc/processor.py builds one ArkaneAdapter over all converged species of the project with reactions=None, which is the thermo calc_type this collapse ran under, so any two isomorphic species anywhere in a project were affected. Its only benefit was suppressing a stderr traceback. Arkane calls save_thermo_lib as the second-to-last statement of execute(), so output.py is already on disk when it raises DatabaseError on a duplicate, and the layer-1 reload in arc/scripts/save_arkane_thermo.py recovers each species' own thermo from output.py without help from the collapse. Removes _dedup_thermo_species_list, _propagate_duplicate_species_thermo, _thermo_duplicate_aliases, their two call sites, and the TestArkaneThermoDedupAndReload tests plus the DEDUP_OUTPUT_PY fixture that existed only for them. Every species now gets its own block in the Arkane input and its own thermo back.
Brings arcbench the parts of 457d3d2 (fix_arkane_thermo_output_reload_main, PR #964) that it lacks, now that the ArkaneAdapter dedup layer is gone and both A+A duplicates carry their own thermo again. save_arkane_thermo.py: _iter_thermo_calls located each thermo() block by regex plus a balanced-parenthesis scan, so a parenthesis inside a string literal - a NASA comment reads "fitted using ..." and Arkane's own comments do contain them - left the scan unbalanced and dropped that species silently, the same null-thermo symptom the fallback exists to prevent. It now walks an ast.parse tree and returns the ast.Call node of every call to the bare name thermo, which cannot be confused by string content and does not mistake the nested thermo= keyword for a call. A file that is not valid Python yields no blocks instead of raising. save_arkane_thermo.py main(): one entry whose thermo cannot be evaluated - a NASA polynomial not valid at 298.15 K, say - raised out of the loop and no thermo.yaml was written at all, losing every other species in the run. Each entry is now evaluated under its own guard, reported on stderr, and skipped. plotter.save_thermo_lib: with both duplicates carrying thermo, the library got two entries whose adjacency list and multiplicity match, and rmgpy.data.thermo.ThermoLibrary.load_entry rejects that pair - the library went from empty to unloadable. save_thermo_lib now applies the same test RMG applies and omits the later entry with a warning. That test already existed on arcbench as processor._thermo_lib_has_isomorph, applied at the single save_thermo_lib call site. Rather than add a second copy, this moves it into save_thermo_lib, where it protects every caller and compares the adjacency lists that are actually written rather than the ARCSpecies.mol objects, so a species carrying an explicit adjlist and no mol is covered too. processor._thermo_lib_has_isomorph and its call site are removed; species_for_thermo_lib feeds nothing but save_thermo_lib, so the move is behaviour-preserving. arcbench spells the per-temperature field thermo_points where main spells it cp_data; arcbench's spelling is kept throughout, including in the tests brought across. Tests: the ast parser (per-species nodes, a paren inside a string literal, an unparseable file), one bad entry not costing the others, end-to-end recovery through the real script in rmg_env with no library present and both duplicates recovering their own thermo, JANAF checks on H2O at 298.15 K and 2000 K against the serialized nasa_high coefficients, the library duplicate guard, and an actual RMG load of a generated library. Adds the arc/testing/statmech/thermo_aa/output.py fixture the script-level tests read.
parse_s_squared took the last line carrying the <S**2>= substring, excluding
only the Initial guess line. In a log produced by a Stable job that line is an
Eigenvector line of the stability analysis, which reports the spin of a
stability-matrix root rather than of the wavefunction:
<Sx>= 0.0000 <Sy>= 0.0000 <Sz>= 0.5000 <S**2>= 0.7536 S= 0.5018
Eigenvector 3: 2.041-A Eigenvalue= 0.0744695 <S**2>=0.791
On the log those lines are taken from it returned 0.791 where the wavefunction
is at 0.7536. Both are plausible doublet values, and the value is carried
through output.yml into the TCKDB spin_diagnostic, so nothing downstream would
have looked wrong. A restricted Stable log was worse: it prints no spin line at
all, yet its eigenvector lines gave a value, so the parser returned a
fabricated diagnostic for a closed-shell reference where its documented
contract is None.
Read the value only from a line that also carries <Sx>=. That substring appears
on the SCF spin line and nowhere else: across the 2,665 <S**2>= lines in the 72
Gaussian logs already under arc/testing -- g03 through g16, opt, freq, scan,
sp, IRC and CBS-QB3 composite jobs -- not one lacks it, so the anchor is a
no-op on every log without a stability block. The other Gaussian spellings of
the quantity carry no <S**2>= substring and were already unread: the guess line
<S**2> of initial guess=, the PUHF table's (S**2,S)=, and the CASSCF
<S**2>(linearized). Where the spin line is absent the parser now yields None
rather than a number read off some other line, which for a diagnostic that
reaches a database is the right direction to fail in.
The regression fixtures are two real stable=(rext,noopt) logs of campaign TS
geometries, byte-identical to the copies on feature_wavefunction_stability_check
which is the branch that produces logs of this shape: an unrestricted doublet
whose roots sit at 0.762 and 0.791 while the reference is at 0.7536, and a
restricted singlet whose only <S**2>= lines are its roots.
(cherry picked from commit ecc755b)
ARC hard-coded the 'default' RMG family set at every entry point that asks which families to consider. That set is RMG's recommended list, which is right for mechanism generation but excludes families ARC's TS adapters support, so a run targeting specific reactions (a benchmark, say) could not reach them at all. A new top-level setting, rmg_family_set, now supplies that value. It defaults to 'default', so an installation that does not set it behaves exactly as before. The setting is resolved inside get_all_families(), which already owned the fallback for its own None default and which every other entry point funnels through: get_reaction_family_products(), check_family_name(), and ARCReaction.get_product_dicts()/determine_family() now simply propagate None to mean "not specified". Resolving it there rather than in each signature is what makes it work: a default argument is evaluated once, when the def executes, so a signature reading the setting freezes it at import and assigning the setting afterwards has no effect whatsoever. That is precisely how this feature was written when it lived only on the benchmark integration branch -- the module constant RMG_FAMILY_SET = settings['rmg_family_set'] used as a default argument -- and it is the kind of defect a reviewer catches on sight, which is the argument for this code having a branch of its own rather than existing only downstream. determine_family()'s shortcut to the cached product_dicts property now keys off rmg_family_set being None rather than equal to 'default', so an explicitly requested set is always honoured even when it matches the configured one. Tests cover both directions: that the shipped default leaves behaviour unchanged, and that changing settings['rmg_family_set'] after import changes which families get_all_families(), check_family_name(), get_product_dicts() and determine_family() consider. The latter fail against the previous implementation. (cherry picked from commit df41bf8)
Cherry-picked from feature_nmd_family_recipe 716ee30. get_reactive_bonds_from_family read product_dicts[0]['r_label_map'] without checking discovered_in_reverse. A reverse-discovered match carries its label map in the product index space, so the returned bonds did not exist in the reactant: a Retroene match on C=C[CH]CCC + CC=CCCC reported three of six bonds on atom pairs that are not bonded, including an H-H pair. Select the first forward-discovered dict of the reaction's family, else return None so the caller falls back to the atom-map-derived bonds. The family filter matters because one reaction's product_dicts can legitimately mix families. get_bond_change_candidates now skips the map-derived candidate when there is no atom map, so a reverse-discovered unmappable reaction yields no candidates instead of raising.
``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 each would break on ``None`` -- ``rdkit_conf_from_mol`` calls ``GetNumConformers()`` past a bare except, goflow_ts calls ``Chem.AddHs`` outside its try, and the xyz branch of ``embed_rdkit`` calls ``GetNumAtoms()``. 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; ``linear_test.py`` produced two. 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. So the exception is rare, is not dominated by a known-benign case, and every occurrence means an unsanitized molecule is handed onward -- a plain ``logger.warning`` is both affordable and warranted. Carbon monoxide does reach this code, contrary to what the removed comment implies, but not by the route that comment suggests. ``get_cheap_conformer`` short-circuits diatomics, yet ``mapping/engine.py``'s ``fingerprint`` has no such guard and reaches it through ``determine_chirality``. A full ``rxn.atom_map`` for ``CO + OH -> HOCO`` nonetheless fires zero warnings. Note also that the trigger is ARC's own carbene special case rather than RDKit: for ``[C-]#[O+]`` the carbon has one lone pair and multiplicity 1, so it is given two radical electrons on top of a triple bond, for an explicit valence of five. The message names the exception class and its text so the next occurrence self-identifies rather than needing this investigation repeated.
``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. The guard is scoped to the ``num_confs`` branch. The xyz branch always adds exactly one conformer, so it cannot be reached there -- which is why ``determine_chirality``, an xyz-path caller, is unaffected. Three callers reach the guarded branch and none is made worse by ``None``. ``get_force_field_energies`` wraps its use in ``if rd_mol is not None``, so it still falls through to its OpenBabel fallback. ``species.get_cheap_conformer`` passes it to ``rdkit_force_field``, which returns empty lists for ``None``. ``mix_rdkit_and_openbabel_force_field`` returns early on ``None`` and therefore skips its OpenBabel fallback, where a conformer-less molecule would previously have fallen through to it -- but that function has no callers anywhere in the repository, tests included, so no live behaviour changes. End-to-end results for the three species above are identical to base.
check_product_isomorphism falls back to comparing the standard InChI and the multiplicity when resonance-aware graph isomorphism fails. Standard InChI carries a mobile-H layer that deliberately collapses tautomers onto one string, so the fallback accepted a family-generated product that is a different molecule from the one the user specified: NNNN=NN (H2N-NH-NH-N=N-NH2) accepted as NNNNN=N (H2N-NH-NH-NH-N=NH) NN=NN (H2N-N=N-NH2) accepted as NNN=N (H2N-NH-N=NH) NC=O (formamide) accepted as N=CO (imidic acid) all three pairs sharing a standard InChI. In benchmark reaction 95 ARC selected that dict as product_dicts[0] and built a TS guess, an atom map and an NMD check for a reaction that is not in the input. The fallback's legitimate job is reconciling Lewis structures perceived from XYZ against those perceived from SMILES, e.g. O=C=C(O)C=O and O=C[C-](O)C#[O+], which no resonance structure of either makes graph-isomorphic to the other. Such pairs differ only in bond orders and formal charges and place every hydrogen on the same heavy atom, whereas tautomers move a hydrogen between heavy atoms. The fallback therefore now also requires the two candidates to be isomorphic under Molecule.is_isomorphic(strict=False), ARC's existing electron-agnostic comparison: VF2 with strict=False compares only Atom.element and ignores bond orders, charges, radical electrons and lone pairs, while explicit hydrogens remain graph vertices, so hydrogen placement is still compared. save_order=True keeps the call from perturbing the atom order of the caller's molecules. Gating instead on the per-heavy-atom hydrogen-count multiset was rejected as insufficient: H2N-NH-N=N-NH-NH2 and H2N-NH-NH-N=N-NH2 are different molecules that share a standard InChI and carry the same hydrogen counts on every heavy atom. The InChI FixedH layer was rejected because ARC's to_inchi has no options passthrough, so it would mean new plumbing through the vendored molecule package for a comparison that graph isomorphism already answers exactly. Because that connectivity comparison is a necessary condition for a match, it is made first and the InChI is generated only for a candidate/species pair it has already admitted. Molecule.is_isomorphic itself checks the fingerprint and the multiplicity before running VF2 and returns False on either mismatch, so it subsumes the separate molecular-formula gate and the separate multiplicity comparison the fallback used to make, and those are dropped rather than duplicated. The InChI comparison itself is unchanged and still required. This is what the ordering costs when it is the other way round. In benchmark run re_run_dmat/kfir_rxn_13108, O=[C]C#CC=O + c1cnon1 <=> O=CC#CC=O + N1=C[C]=NO1 (H_Abstraction) the template generates [N-]1O[NH+]=C=C1, a C->N tautomer of the oxadiazole reactant. It is valence-legal, so RMG builds it and to_smiles() succeeds, but its cumulated C=C=N inside a five-membered ring is geometrically impossible and neither RDKit nor OpenBabel will make an InChI for it. It shares a formula and a multiplicity with c1cnon1, so the old formula gate let it through to to_inchi, where Molecule.to_inchi's logger.exception and translator._write's logger.error each fired before check_product_isomorphism swallowed the exception and correctly dropped the candidate. ARC resolves the family four times per reaction, so a healthy run logged eight ERROR blocks with full tracebacks, roughly 24 lines, for a candidate no InChI was ever needed to reject. Per family resolution the count goes from 30 InChI generations and 2 failures to 0 and 0, with n_dicts, the resolved family, the products and both label maps unchanged. The p_species InChIs are also no longer precomputed for the whole list behind one try/except that returned False for the entire call. That made a single InChI-hostile user-specified product reject every candidate and silently kill family recognition for the reaction, which is the same defect as above pointed at the user's input instead of the template's output. The lookup is now per pair, so a molecule no backend can convert only removes itself from consideration, and results are memoized per call, including failures, so no molecule is converted twice. The old formula gate's spc.mol is None guard is dropped rather than carried over: check_product_isomorphism dereferences spc.mol unconditionally when it augments singlet biradicals, well before the fallback is reached, so on both sides of this change a None mol raises AttributeError there and the guard in the fallback was unreachable. Neither comparison modifies the molecules it is given, which is what makes the reordering a reordering of two independent conditions rather than a change in what the function does to its arguments. is_isomorphic(save_order=True, strict=False) was measured to leave atom order, charges, radical electrons, lone pairs and bond orders untouched. to_inchi is the one that does mutate: it routes through to_rdkit_mol(sanitize=True) with save_order defaulting to False, so sort_atoms permutes the vertices of the molecule it is handed and never restores them. On the accepted pair O=C[C-](O)C#[O+] / O=C=C(O)C=O it turned the caller's product from O C C O C O H H into O O O C C C H H, and it did the same to the user's spc.mol. _get_inchi therefore converts a deep copy, as arc/output.py already does at its own to_inchi call site. Over 16 molecules, including the anions to_inchi cannot convert at all, the copy yields a byte-identical result in 16 of 16 for +0.015 ms (0.179 to 0.194 ms median), and the atom order of both products and p_species is left untouched for every input, matching or not. A 26-case sweep over matching, non-matching, tautomer, biradical, multiplicity and InChI-hostile inputs gives identical verdicts, with the single intended exception of the InChI-hostile user product described above, which goes from a wrong False to True. Searched for an existing helper before adding one: arc/output.py has a private _safe(fn, default) with the same shape, but it is an output-layer private and importing it into arc/family would invert the layering, and arc/species/converter .py::pybel_to_inchi converts a pybel molecule rather than guarding ARC's own to_inchi. No memoizing or failure-tolerant InChI accessor exists, so _get_inchi is added beside its concept. Seven regression tests: the tautomer pairs above and the equal-hydrogen-count pair must be rejected (both verified failing before this change), the Lewis-structure pair must still be accepted through the fallback, which the test pins by asserting no resonance structure of either candidate is graph-isomorphic to the other, the benchmark's [N-]1O[NH+]=C=C1 against c1cnon1 must be rejected without to_inchi being called at all (verified failing before this change with 1 != 0), [CH3+] must not match [CH3-] and C=[CH+] must not match C=[CH-], pairs the connectivity comparison and the multiplicity both admit so that the /q layer of the standard InChI is the only thing separating them, _get_inchi must give each molecule its own cache entry, and check_product_isomorphism must leave the atom order of both its arguments untouched on a pair the fallback accepts. The last three each fail against a mutant of this implementation that respectively drops the InChI comparison, collapses the cache onto one key, or converts in place; the nine tests the file already had survive all three mutants.
charge_filtration, find_unique_sites_in_charged_list and stabilize_charges_by_proximity identify atoms by sorting_label, which the isomorphism machinery leaves either unset or holding a stale permutation that no longer matches the vertex order. Under save_order the structures of one species arrive in a mixed state -- for 4-nitrophenoxy, 12 of the 18 structures reaching charge_filtration carry the unset value and the other 6 a stale permutation, against 18 of 18 correctly labelled without save_order. The proximity heuristic only counts a charged pair when atom2.sorting_label > atom1.sorting_label, so it measures a real distance for the few labelled structures and zero for the rest, then pops everything above the minimum. For charged aromatic radicals such as 4-nitrophenoxy that removed every aromatic structure. Use the atom's position in mol.vertices instead, via a new get_atom_indices, and drop the label comparison in stabilize_charges_by_proximity for itertools.combinations, guarding the disconnected case where find_shortest_path returns None. Over a 189-species corpus this takes the number of species whose distinct structures differ between the two save_order settings from 10 to 0, and removes a run-to-run nondeterminism: 4-nitrophenoxy under save_order=True returned 9 or 7 structures depending on the process and now returns 11 in every run, matching save_order=False. It also restores the three worked examples in this module's own docstring, all of which save_order=True contradicted: NO2 2 -> 4, CH2NO 2 -> 3, NH2CHO 1 -> 2. No structure is lost anywhere in the corpus, under either save_order setting. Two further corrections, both needed to keep that true: Parenthesise the multiple bond check in charge_filtration. index2 > index1 and bond.is_double() or bond.is_triple() parses as (A and B) or C, so a triple bond satisfied the condition from both sides and the reversed index pair was recorded as well (N#S recorded [(0, 1), (1, 0)]). find_unique_sites_in_charged_list, the only reader of mul_bond_sorting_list, looks up ordered pairs only, so the reversed entries were never queried and this changes no results; the sibling check in find_unique_sites_in_charged_list is already parenthesised, so the two read as if they disagree. Compare the similar-charge distance against the similar-charge maximum in stabilize_charges_by_proximity. The second pass tested distances[0], the cumulative opposite-charge distance, against the maximum of distances[1]. Rule 4 keeps the structures whose like charges are furthest apart, so it must test distances[1]. The mismatch was unreachable while the heuristic was inert; computing real distances makes it pop every structure of a salt whose opposite-charge pairs are all cross-fragment, so [Li+].[Li+].[O-][O-] and O=C([O-])[O-].[NH4+].[NH4+] raised ResonanceError. Corrected, both return a structure under either save_order setting, and no other species in the corpus changes.
Covers the nitroaromatic radicals whose aromatic structures were dropped under save_order and the NO2 / CH2NO / NH2CHO examples this module's docstring documents, asserting the same structure count and the same number of aromatic structures for either save_order setting. Covers the salts whose charged atoms sit in different molecular fragments, which raised TypeError from the charge proximity heuristic or were filtered away entirely. Adds a unit test for get_atom_indices.
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.
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.
…ion time timedelta_from_str() claimed to invert str(datetime.timedelta) but implemented a '1hr2m3s' grammar instead. Every group in that regex was optional, so the pattern also matched the empty string: any input in the real str(timedelta) form matched zero characters and the function returned timedelta(0). TSGuess.from_dict() is the only caller, and it feeds it exactly that form - TSGuess.as_dict() writes str(self.execution_time), and the gcn, goflow, kinbot and rits scripts all write str(datetime.datetime.now() - t0). So every TS guess's execution time silently became zero on restart. ARC's own restart fixture arc/testing/restart/5_TS1/restart.yml stores '0:00:05.357294' for seven heuristics guesses, all of which restored as zero. TS guess cost per method is a reported benchmark quantity, so this corrupted data rather than only a log line. Both grammars are kept. Nothing in the repository or its history produces or persists the '1hr2m3s' form - the regex arrived with the function in bd168da and never had a producer - but the two grammars are disjoint (one uses colons, the other letters), so accepting both costs nothing and avoids silently removing behaviour from a public helper in arc/common.py. The str(timedelta) grammar is tried first and is anchored at both ends, which is what stops the empty match. Unparseable input now returns None with a logged warning rather than raising. The caller assigns the result straight to TSGuess.execution_time, where None is already a first-class value - as_dict() guards it with `is not None` and scheduler.py str()s it - whereas raising would abort restart of an otherwise valid project. That is not hypothetical: arc/testing/restart/2_restart_rate/ restart.yml persists a legacy execution_time of '0', which no duration grammar accepts. The warning is what keeps the failure visible; returning a plausible-looking zero is what hid this bug. An exact zero stays distinguishable from a parse failure: '0:00:00' parses to timedelta(0), unparseable input yields None. The existing test asserted only isinstance(result, datetime.timedelta), which timedelta(0) satisfies, so it passed forever while enshrining the bug. It now asserts parsed values, and a round-trip test covers sub-second, multi-day, zero and negative durations. Two rendering helpers are added here as well, for the TS guess report that the following commit rewrites as a table. format_table() sizes each column to its own widest entry, header or cell, and supports a multi-line header so a units line can sit under a title. It validates its inputs and raises InputError for a ragged row, an alignment character other than '<', '>' and '^', and a non-string cell, rather than letting those surface as a TypeError or a ValueError from inside the renderer; a column whose multi-line header is empty and which has no rows now renders at width zero instead of raising from max() on an empty sequence. Its docstring states that widths are counted in characters, which is the contract a caller needs in order to know that alignment holds for single-width text; no display-width measurement is implemented, because the only cell that could carry a full-width or combining character is Status, built from tsg.errors, which is ASCII as an ESS emits it. format_duration() reports a duration in the largest unit it fills, to one decimal place ('3.4 s', '47.2 m', '13.1 h', '2.1 d'). Most TS guesses finish in seconds and per-method guess cost is a reported benchmark metric, so the report has to resolve a 3.4 s heuristics guess from an 18.1 s AutoTST one, which a whole-minute format cannot. One decimal place in a self-naming unit keeps that resolution and spans seconds to days while staying short enough to sit in a column without a unit line of its own. The unit loop covers the three bounded units and days are the terminal return after it, rather than a fourth entry whose limit is a None sentinel: the sentinel entry always matched, so the function could only return from inside the loop even though it is annotated `-> str`, which is what CodeQL's py/mixed-returns flags. Structuring it as "three bounded units, then days as the unbounded fallback" removes the implicit fall-through without leaving a line after the loop that can never execute. time_lapse() is corrected in the same file. It computes its day count with divmod() on a float, so `str(d)` rendered it as '1.0' and the function returned '1.0 days, 06:00:00' - which matches neither the "D HH:MM:SS" format its own docstring claims nor Python's own str(timedelta). Formatting the count with '.0f' makes it a whole number. Sub-day output is untouched. This also removes an inconsistency inside the module this commit restructures: arc/common.py now holds one duration parser, and timedelta_from_str(), newly anchored, rejected the day form that time_lapse() produced. Nothing feeds one into the other today, so the defect was latent, which is also why no test caught it - there was no coverage above 24 hours at all. There is now, and it asserts the round trip through timedelta_from_str() as well as the rendered string. Searched arc/common.py, arc/plotter.py and the rest of arc/ for an existing column, table or padding helper (def .*(pad|align|column|width|table|format), ljust/rjust/center, tabulate/PrettyTable, "max(len(") and for a duration formatter (time_lapse, timedelta_from_str, convert_to_hours). No table helper exists; the only comparable code is the Arkane thermo table in arc/statmech/arkane.py, which hand-rolls its widths inline. Rather than add a second copy of that, the renderer goes in arc/common.py, which is where the duration formatter belongs too. arkane.py is left alone because converting it would change thermo log output this change is not otherwise concerned with; it is the obvious next caller. format_duration() does not carry a duration grammar of its own. It delegates the string case to timedelta_from_str(), so the module holds exactly one parser for the str(timedelta) form and the two cannot drift apart - which they already had: a parser written privately against the broken timedelta_from_str() allowed no sign on the days field and so failed on '-1 day, 23:59:59', which timedelta_from_str() reads. Delegating is only correct once timedelta_from_str() is fixed; against the old implementation every colon-form duration would have rendered as '0.0 s'. The contract seam is preserved in both directions. timedelta_from_str() returns None for unparseable input and format_duration() returns '' for it, so the composition still yields ''. An exact zero stays distinguishable from a failure: '0:00:00' renders '0.0 s', garbage renders ''. Negative durations now parse, where the private regex rejected them outright, and format_duration() still reports them as '' - a negative guess duration is not worth displaying, and that was the behaviour before. An empty or blank string is treated as an absent duration and short-circuits before the parser, so a missing execution time does not log a parse warning once per report row; a non-blank string that is genuinely malformed still warns, which is the signal worth keeping.
The per-guess lines logged by determine_most_likely_ts_conformer() were prose
with embedded labels, and they did not line up: three of the four fields ahead
of the free-text tail were rendered at their natural width. The method string
is 7-24 characters, longer still when a clustered guess adds an "(also: ...)"
suffix; the execution time varies; and the index format spec hard-coded a
width of 2, so an index of 100 or more shifted the whole line. Only the
relative energy was padded, at a fixed width of 8 that was both wider than the
data needs and able to overflow.
The block is now a table with a header, a rule, and one row per guess:
TS Guess, Method, Rel. Energy (kJ/mol), Guess Time, Img Freq (cm-1), and
Status. Each column is sized to its own widest entry, computed over the
guesses that are actually reported, i.e. those that pass the "success and
energy is not None" filter, so a guess that is filtered out cannot leave a
permanently over-wide column. Sizing them needs the rendered cells before the
first line is emitted, so the guesses are collected in the existing loop,
which already mutates tsg.energy into a relative energy, and logged in a
second pass.
The whole table is emitted before any structure is drawn. plotter.draw_3d()
opens with an unconditional logger.debug('not drawing 3D!'), so drawing each
guess right after its own row put that line between the rows and destroyed the
alignment whenever ARC runs at DEBUG - and verbose is a documented user-facing
ARC input, persisted to the restart dict when it is not INFO, so DEBUG is a
supported mode rather than a developer-only one. Splitting the loop keeps the
table contiguous. The draws themselves are unchanged: one per reported guess,
in row order, still with method='draw_3d', because for a TS only draw_3d() may
be used - show_sticks() infers connectivity and gets it wrong for a structure
with partial bonds.
The guess time cell comes from format_duration(), which reports the duration in
the largest unit it fills to one decimal place. Most TS guesses finish in
seconds and per-method guess cost is a reported benchmark metric, so the column
has to resolve a 3.4 s heuristics guess from an 18.1 s AutoTST one; the previous
code truncated str(tsg.execution_time) at one decimal place, which was legible
only because it happened to be sub-minute, and a whole-minute format would
collapse every row of a real block to one value.
The Status column carries tsg.errors, which was previously appended to the end
of the prose line, and is added only when a reported guess actually has an
error: errors are recorded exclusively on guesses with success False (see the
loop at the end of troubleshoot_ess), which this block filters out, so the
column would otherwise always be blank. The imaginary frequencies, previously
spelled out mid-sentence, are now just the Img Freq column.
The relative-energy conversion is also made idempotent, which is a pre-existing
defect this commit fixes rather than one it introduces - the same subtraction,
with the same asymmetry, is on main. e_min is the lowest energy of ANY guess,
successful or not, but `tsg.energy -= e_min` was applied only to the successful
ones. A second invocation for the same label therefore saw a set in which the
successful guesses already held relative energies while the unsuccessful ones
still held absolute ones; if the global minimum sat on an unsuccessful guess,
e_min stayed at that absolute value and every successful energy shifted again.
Measured on main, for guesses at +10 and +20 that succeeded and one at -100 that
did not: [110.0, 120.0, -100.0] after the first call, [210.0, 220.0, -100.0]
after the second. Repeat invocation is reachable from switch_ts() and from two
call sites in arc/job/pipe/pipe_coordinator.py.
The subtraction is now applied to every guess that has an energy. This keeps the
reference point exactly where it was - the lowest energy of any guess - so no
energy reported on a first invocation changes, which matters because those
numbers are physical and feed the selection. It also makes the set minimum
exactly zero, so a second invocation subtracts zero and is a no-op. Computing
e_min over only the successful guesses would have been idempotent too, but it
moves the reference point whenever the global minimum sits on an unsuccessful
guess, and so changes reported energies; that was rejected for exactly that
reason.
The values newly mutated are the energies of unsuccessful guesses. Two things
read them. plotter.save_conformers_file() takes the energies of all guesses and
re-baselines them against their own minimum, so a uniform offset leaves its
output unchanged; it was previously handed a mixture of relative and absolute
values, and now receives one consistent baseline. TSGuess.almost_equal_tsgs()
compares two energies with isclose(abs_tol=0.1), a difference, which a uniform
offset also leaves unchanged. as_dict() persists the energy, so a restart now
restores an already-relative set whose minimum is zero, on which the conversion
is again a no-op.
TSGuess.from_dict() parsed ts_dict['execution_time'] near the top and then, for a 'user guess' method, overwrote the result with timedelta(seconds=0) at the bottom. Now that an unparseable duration is reported rather than silently read as zero, that ordering makes ARC warn about values it throws away: loading arc/testing/restart/2_restart_rate/restart.yml, whose four TS guesses are all 'user guess' entries carrying a legacy execution_time of '0', emitted three "Could not interpret '0' as a time delta" warnings per restart for a field whose restored value cannot depend on it. The method is now resolved before the execution time, and the execution time is not parsed at all when the method is a user guess, since the later branch is the authority on that value. Nothing else moves, and the warning is untouched for every guess whose execution time is actually kept - an unparseable duration on a kinbot or heuristics guess is a real loss of benchmark data and still says so. TSGuess.as_dict() writes str(self.execution_time) and from_dict() reads it back, so that pair is the path on which every TS guess's execution time was silently reset to zero on restart. Nothing tested the pair itself: the parser had a unit test that asserted only isinstance(result, datetime.timedelta), which timedelta(0) satisfies, so the defect survived a green suite. The new TestTSGuess cases round-trip a sub-second duration, a multi-day one and an exact zero through as_dict()/from_dict() and assert the restored value equals the original, load the restart fixture and assert it produces no warning, and assert that a non-user-guess entry with the same unparseable value still warns. That pins the behaviour at the level a restart actually exercises rather than at the level of the regex.
`pyproject.toml` runs the suite with `--dist=worksteal`, which distributes
individual tests rather than whole files. Every worker that receives any test of
a class therefore runs that class's `setUpClass` *and* its `tearDownClass`.
`TestTSChecks` builds its fixtures inside two directories under
`ARC_PATH/Projects/`, named `arc_project_for_testing_delete_after_usage4` and
`..._usage5`, and rmtrees both in `tearDownClass`. So the first worker to finish
its share of the class deleted the `freq.out` files that another worker's
still-running `test_compute_rxn_e0` and `test_check_rxn_e0` had just copied
there. Arkane then found no input and left every E0 as None, which surfaced as a
TypeError subtracting a float from None, and as `ts_checks['E0']` being None.
The same two directory names are hard-coded in `scheduler_test.py` and
`species_test.py`, which rmtree them as well, so the collision crosses modules
as much as it crosses workers.
`TestNMD` and `TestPlotter` collide the same way. `TestNMD` builds a Gaussian job
adapter under `Projects/tmp_nmd_project` and rmtrees it, so a worker that
finished its share first removed the tree while another worker's `setUpClass` was
still building into it, raising FileExistsError for every test that worker had
left. `TestPlotter` uses `Projects/arc_project_for_testing_delete_after_usage`,
a sibling of `..._usage4` and `..._usage5` rather than an ancestor, so the three
rmtrees do not nest and each name is scoped on its own; there another worker's
`tearDownClass` removed the `N4H6.yml` copy that
`test_augment_arkane_yml_file_with_mol_repr` had just made.
`TestPlotter` also wrote three outputs into `ARC_TESTING_PATH` itself, alongside
the input data it reads: `bde_report_test.txt`, `irc/rxn_1_irc_animation.out`,
and the `water.log` / `acetylene.log` / `N-Valeric_Acid.log` that
`make_multi_species_output_file` slices out next to its input path. The last
three are asserted present by `test_make_multi_species_output_file` and asserted
absent by `test_delete_multi_species_output_file`, which holds while the two run
in sequence but not while they run concurrently on separate workers. All of these
now go under the worker's own project directory, which `tearDownClass` already
removes wholesale, so its `files_to_remove` list is empty and gone.
Deriving the directory from `PYTEST_XDIST_WORKER` gives each worker its own tree
and leaves serial runs unchanged. The two helpers live in `arc/common.py`
because that module already owns `ARC_PATH` and `ARC_TESTING_PATH`; both are
used directly, one for a project name and one for a path, and the private copy
in `functional/restart_test.py` is folded into them rather than left as a third.
Per-test fixtures were the alternative and were rejected, but not on cost.
`setUpClass` measures 0.21 s, not the four seconds an earlier draft of this
message claimed, so twenty-five per-test fixtures would add about five seconds of
serial suite time rather than ninety; `pytest arc/checks/ts_test.py --durations=0
--durations-min=0.0` charges class setup to the first test's setup phase, and the
4.4 s of the earlier draft was `test_compute_rxn_e0`'s *call* duration. The
reason per-test fixtures do not work is that they can only scope a directory
within the class that declares it, while every collision here is between classes:
`scheduler_test.py` and `species_test.py` rmtree the two names `ts_test.py` uses,
and `TestNMD` and `TestPlotter` each own their tree outright.
Measured with `taskset -c 0-3 python -m pytest <target> -n 6 --dist worksteal`:
arc/checks/ts_test.py before: 2 failed, 23 passed
1 failed, 15 passed, 9 errors
2 failed, 23 passed
2 failed, 16 passed, 7 errors
1 failed, 16 passed, 8 errors
2 failed, 17 passed, 6 errors
after: 25 passed, in all six runs
arc/checks/nmd_test.py before: 14 passed, 10 errors
14 passed, 10 errors
15 passed, 9 errors
after: 24 passed, in all six runs
arc/plotter_test.py before: 1 failed, 8 passed
2 failed, 7 passed
1 failed, 8 passed
after: 9 passed, in all six runs
arc/checks/ before: 51 passed
51 passed
51 passed
51 passed
43 passed, 8 errors
51 passed
49 passed, 2 errors
49 passed, 2 errors
49 passed, 2 errors
after: 51 passed, in all nine runs
A single test file fails far more readily than a whole directory: with 24 or 25
tests to spread over six workers, worksteal splits the class every time, while
`arc/checks/` is red in four runs of nine. Across all 2800 tests of the CI
invocation `pytest arc/ -n auto --dist=worksteal` the chunks are large enough
that a class normally lands on one worker, and that invocation did not reproduce
any of these failures. The full suite is also unchanged before and after this
commit: the same five deterministic `torch_ani_test` failures from a local
environment gap, and the same intermittent
`adapter_test::test_determine_job_status`, which is a separate shared-directory
race not addressed here.
`TestARC` and `TestScale` both delete a fixed project directory under
`ARC_PATH/Projects` from a class-level fixture, and `--dist=worksteal` runs those
fixtures on every worker that receives any test of the class. The deletion is
therefore not paired with the test that created the tree.
`TestARC` rmtrees six project directories in `setUpClass` *and* in
`tearDownClass`, so a worker starting or finishing its slice removes the trees
the other workers are running in. `ARC.__init__` creates the project directory
and then opens `arc.log` inside it, which is where the race lands:
FileNotFoundError: [Errno 2] No such file or directory:
'.../Projects/unit_test_specific_job/arc.log'
FileNotFoundError: [Errno 2] No such file or directory:
'.../Projects/arc_test/arc.log'
FileNotFoundError: [Errno 2] No such file or directory:
'.../Projects/arc_model_chemistry_test/arc.log'
`Projects/test` and `Projects/arc_test` are each built by several tests of the
class as well, and `test_determine_model_chemistry_and_freq_scale_factor` reads
back the `arc.log` that `ARC(project='test')` wrote there. `ARC` derives the
project directory from the project name when no directory is given, so the names
themselves are scoped rather than the paths; the two restart dictionaries that do
pass a path explicitly are scoped through `get_test_project_directory`. The
substring assertion on `arc2.project_directory` still matches, because the worker
ID is appended rather than substituted.
`TestScale` builds `Projects/scaling_factors_arc_testing_delete_after_usage` in
`test_summarize_results` and rmtrees it in `tearDownClass`, so a worker holding
only `test_get_species_list` deletes the `scaling_factors_0.info` file that
`test_summarize_results` asserts on. That window is narrow - the file is written
and read back in consecutive statements - and 20 runs did not catch it; the
directory is scoped because the shape of the exposure is identical, not because
it was observed to fail.
Measured with `taskset -c 0-3 python -m pytest <target> -n 6 --dist worksteal`:
arc/main_test.py before: 11 passed
1 failed, 10 passed
11 passed (x11)
3 failed, 8 passed
11 passed (x8)
after: 11 passed, in all six runs
arc/utils/scale_test.py before: 4 passed, in all twenty runs
after: 4 passed, in all three runs
Serial results are unchanged: 11 passed and 4 passed respectively.
The adapter test modules build their scratch project directory from a fixed path
under `arc/testing/` rather than under `ARC_PATH/Projects`, so they are exposed to
the same `--dist=worksteal` race as the project directories: `setUpClass` runs
once per worker that receives any test of a class, the teardowns rmtree that tree,
and a worker finishing its slice deletes input files out from under another
worker's running test. `adapter_test.py` and `orca_neb_test.py` register the
removal with `addClassCleanup`, `common_test.py`, `gaussian_test.py`,
`molpro_test.py` and `orca_test.py` with `tearDownClass`; all six run per worker.
`common_test.py` and `gaussian_test.py` also happened to pick the same directory
name, `test_GaussianAdapter`, which collides between the two modules however the
tests are distributed. Scoping alone does not separate them, since both would
resolve to the same worker-suffixed path, so `common_test.py`'s two directories
are renamed to `test_GaussianAdapter_common` and `test_MolproAdapter_common`.
The worker suffix comes from `get_test_project_name` in `arc/common.py`, the same
helper the `Projects` directories use, composed with `ARC_TESTING_PATH` at the
point of use. No second reader of `PYTEST_XDIST_WORKER` is introduced.
Measured with `taskset -c 0-3 python -m pytest <the six modules> -n 6 --dist
worksteal`:
before: 3 failed, 62 passed, 9 errors
1 failed, 66 passed, 7 errors
3 failed, 71 passed
6 failed, 68 passed
2 failed, 58 passed, 14 errors
4 failed, 70 passed
after: 74 passed, in all six runs
Serially the same six modules give 74 passed before and after. This also covers
the intermittent `adapter_test::test_determine_job_status`, which reads the PBS
time-limit fixture that `setUpClass` copies into
`test_JobAdapter_ServerTimeLimit`.
…s with arcbench Three integration fixes for main-based commits that assumed a tree arcbench does not have. Each is an arcbench-only adjustment, not a change to what the picked commits do. arc/main_test.py: the main-based scoping commit dropped ARC_PATH from the arc.common import because nothing on main still used it. arcbench carries an extra test class, TestRestartRoundTrip, whose tearDown joins ARC_PATH, so every test in it failed with NameError. The import is restored. That class remains unscoped, like the other arcbench-only Projects sites in scheduler_test.py. arc/scheduler.py: report_omitted_ts_guesses(), which exists only on arcbench, now runs after the guess table and its structure drawings rather than between them, so the table block is emitted uninterrupted and the omitted-guess line still reads as being about the guesses listed above it. arc/scheduler_test.py: TestSchedulerTSGuessReportAlignment treats every non-blank line logged after the guess block header as a table row, which is true on main, where no omitted-guess line exists. The helper now skips that one line so the column assertions apply to the table alone. No assertion is relaxed and no case is dropped.
_generate_resonance_structures() accepted a save_order argument, used it for its own isomorphism checks, and then invoked every generation method as method(molecule), so each dispatched method fell back to its own save_order=False default. On the keep_isomorphic=True path this silently discarded the Clar structures of charge-separated polycyclic aromatics. 1-nitronaphthalene returned 3 structures instead of 7, 1-nitroanthracene 3 instead of 9, 1-nitropyrene 3 instead of 5, and 1-azidonaphthalene 2 instead of 4. The deletions are the dominant resonance contributors by Clar's rule, and they were selected by an accident of bookkeeping. filter_structures() runs charge_filtration(), which calls stabilize_charges_by_proximity() (arc/molecule/filtration.py:300); that heuristic ranks structures by walking pairs of charged atoms under the guard "atom2.sorting_label > atom1.sorting_label" (filtration.py:314). sorting_label defaults to -1 and is assigned by Molecule.sort_atoms(), which was reached only from to_rdkit_mol(save_order=False) (arc/molecule/converter.py:34) by way of get_aromatic_rings() - that is, only from inside the Clar step. So the Clar structures were the only ones whose labels were initialised, they were the only ones the guard let the heuristic measure, and it popped exactly them. Instrumenting 1-nitronaphthalene: 8 structures reach stabilize_charges_by_proximity(), the 4 with initialised sorting labels are precisely the 4 one-sextet Clar structures, and precisely those 4 are popped; none of the 3 survivors is a Clar structure. With save_order honoured no structure carries an initialised label, nothing is popped, and 4 of the 7 survivors are Clar structures. The loss also violated the de-duplication invariant n(keep_isomorphic=True) >= n(keep_isomorphic=False): 1-nitronaphthalene gave 3 < 4, 1-nitroanthracene 3 < 5 and 1-azidonaphthalene 2 < 4. The invariant is restored for all three. Thirteen distinct methods are reachable through this dispatch loop and 2 of them accept save_order: generate_clar_structures() and generate_aromatic_resonance_structure(), both dispatched by generate_resonance_structures() itself. The other 11 take only the molecule, so a bare method(molecule, save_order=save_order) would raise TypeError. generate_optimal_aromatic_resonance_structures() also accepts save_order but is not reachable here: it appears only in populate_resonance_algorithms(features=None), whose sole consumer is generate_isomorphic_resonance_structures(), and that invokes algo(isomer) directly rather than through this loop. _SAVE_ORDER_METHODS lists all three algorithms that accept the argument, not only the 2 this loop reaches, so that it reads as a complete inventory. Membership is a module-level tuple rather than a runtime signature inspection: test_save_order_aware_algorithms() derives both sides from the actual signatures and asserts set equality, so an algorithm that gains or loses save_order fails the test rather than drifting silently. The tuple also fails closed - a stale entry raises TypeError or reddens CI, whereas a signature probe that stopped returning real signatures, through a Cython binding=False switch or a decorator wrapping a generator, would answer False for every method and quietly reinstate this bug. This mirrors the fix for the identical defect in RMG-Py, which this module is vendored from and which still carries it at rmgpy/molecule/resonance.py:311, so the two implementations do not conflict on a re-vendor. The atom order matters because several consumers of the resonance structures are positional. ARCSpecies.set_mol_list() (arc/species/species.py:1084-1091) repairs the order with order_atoms_in_mol_list(), which is not a reliable backstop: update_molecule() rebuilds both molecules with every bond single and drops charges, radicals and lone pairs, so the isomorphism it solves sees only elements and connectivity. The two terminal nitro oxygens of 1-nitronaphthalene are equivalent under that reduction, so the repair can satisfy itself with a graph automorphism instead of the identity. Over 12 permuted input orders it reported success every time, and in 5 of them it left 2 of the 4 structures holding the O- at the index where the reference holds the neutral O=. Elements per index still matched, so no downstream element-and-connectivity check could notice. With save_order honoured the structures arrive already ordered and the repair is a no-op in all 12. set_mol_list() also replaces the whole resonance list with [self.mol] when the repair returns False (arc/species/species.py:1090-1091), so re-sorted structures could collapse mol_list to a single entry and cost rotor and conformer coverage. The other positional consumers are arc/species/conformers.py:222 and ARCSpecies.reconcile_mol_multiplicity() (arc/species/species.py:1534-1536), which adopts resonance[0]. Index 0 is not necessarily the input molecule: mark_unreactive_structures() promotes the input to index 0 only if it survived filtration, and otherwise appends it at the end with reactive=False. C[S+]([O-])SC and [O-][S+]=O both leave the input at index 1. ARCReaction.get_changed_bonds() is a fourth positional consumer, but branch fix_changed_bonds_resonance fixes it at the caller by locating its atoms via atom.id, so once both changes are on main they overlap completely at that one call site. The independent value of this change is the other three consumers and the restored Clar structures. filtration.filter_structures() is still called with a hard-coded save_order=True at generate_resonance_structures():281 rather than with the caller's value. That argument is inert there: filter_structures() only forwards it to mark_unreactive_structures(), which spends it on an is_isomorphic() call over deep copies, so it cannot reach the returned structures. Passing the caller's value instead produces no difference at all over 12 molecules x 2 caller values. ARC diverges from RMG-Py at this line and the divergence is left alone rather than churned. The structure sets are otherwise unchanged for aromatic species. Verified over aliphatics, aromatics, heteroaromatics, polycyclic aromatics and radicals: with keep_isomorphic=True the atom-ID-keyed fingerprints of the generated structures are identical for both values of save_order. That parity is not yet universal, and the remaining gap is a separate defect this change does not address. Vertex.sorting_label initialises to -1 and filtration.py compares labels with a strict >, so the two sorting_label-dependent heuristics see nothing to compare when no structure has been sorted. A handful of charged non-aromatics still return fewer structures with save_order=True than without -- [O]N=O 4 vs 2, C=N[O] 3 vs 2, [CH2]N=O 3 vs 2, NC=O 2 vs 1 -- and that behaviour is byte-identical on main, so it is pre-existing rather than introduced here. With keep_isomorphic=False the isomorphic de-duplication keeps whichever of two symmetry-equivalent structures happens to be generated first, so for naphthalene the representative of one isomorphism class differs between the two atom orders while the counts and the isomorphism classes agree; that is pre-existing behaviour and is unchanged on main.
…tion The existing save_order tests use monocyclic aromatics, which never reach generate_clar_structures(), so they passed while the argument was being dropped. test_resonance_of_polycyclic_aromatics_without_changing_atom_order() covers naphthalene, 1-ethynylnaphthalene and anthracene. It pins the structure counts (3, 4 and 4 at keep_isomorphic=False), asserts that save_order=True leaves the atom IDs, elements and neighbour sets in the input order, and asserts that save_order=False does re-sort at least one structure, which is what fails on main. Structure-set preservation is the property that actually guards the change, and it is asserted on the atom-ID-keyed fingerprint - sorted (id, element, charge, radical electrons, lone pairs) per atom plus sorted (lower id, higher id, bond order) per bond - so it is independent of the atom order it is meant to police. It is asserted as exact set equality between save_order=True and save_order=False at keep_isomorphic=True. At keep_isomorphic=False the isomorphic de-duplication keeps whichever of two symmetry-equivalent structures is generated first, so for naphthalene the representative of one isomorphism class legitimately differs between the two atom orders; that path is therefore asserted as equal counts plus a pairwise isomorphism match instead. The weakening is confined to the symmetric case and is pre-existing behaviour on main. test_clar_structures_of_charge_separated_polycyclic_aromatics() is the regression test for the discarded Clar structures. 1-nitronaphthalene yields 7 structures at keep_isomorphic=True against 3 on main, and the test also pins the de-duplication invariant n(keep_isomorphic=True) >= n(keep_isomorphic=False), which main violates at 3 < 4. SaveOrderMethodsTest covers _SAVE_ORDER_METHODS, which is what decides whether save_order reaches a method at all. Both sides of the comparison are derived from the real signatures - the dispatchable algorithms being populate_resonance_algorithms() plus generate_aromatic_resonance_structure(), which is only ever reached through the hardcoded single-element method_list in generate_resonance_structures() - so neither direction of drift can pass. An algorithm that gains save_order without being listed would keep running on its own save_order=False default, and one that is listed but no longer accepts it would raise a TypeError that Molecule.generate_resonance_structures() catches, collapsing the result to a single structure. Dropping a member from the tuple and adding a non-accepting one were both confirmed to fail the assertion.
…t neighbour The bond-list comparison is the only tier permitted to write ts_checks['IRC'] = False, and it called get_bonds_from_dmat with the default n_fragments=1. In that mode every hydrogen is bonded to its absolute nearest neighbour with no distance test at all, so a dissociating H is bonded back to the fragment it left and the endpoint re-perceives as the un-dissociated species. _perceive_irc_fragments in the same module already passed n_fragments=0 to avoid exactly this; the tier treated as inconclusive used the safe setting while the authoritative one did not. Measured on correct IRC endpoints: O(3P) + H2 <=> OH + H and S(3P) + H2 <=> SH + H were both recorded False and are now True. The r3_16 product-to-product true negative is unaffected. Found independently by a code review and a chemistry review of PR #937, where this lands as part of 7bbefee.
check_irc_species_and_rxn wrote ts_checks['IRC'] = False as soon as the molecular-graph isomorphism comparison failed, then logged that it was falling back to the bond-list comparison - so it recorded a verdict it did not trust, and left it standing if the fallback could not run. That tier's negatives are not trustworthy. _perceive_irc_fragments passes no multiplicity, so the electronic state is guessed from geometry: singlet CH2 and NH are perceived as triplets, O(3P) and S(3P) as singlets, triplet nitrenes and singlet O2 likewise, and NH4+ + H2O has its charge placed on the wrong fragment. Each is a chemically correct endpoint pair recorded as a failure. The isomorphism tier is now advisory - a match still returns True immediately, a mismatch falls through to the bond-list comparison, and the verdict stays None if that comparison cannot be carried out. The bond-list comparison remains the only place a False is written. Nothing on this branch enforces the verdict, so this changes no run outcome. It changes what is recorded in restart.yml, which is what any re-scoring of completed reactions reads. Found by the chemistry review of PR #937, where it lands as part of 7bbefee.
Sync arcbench with origin/main d033cbb (133 commits). 36 files conflicted. Resolution principle: main's structure where main refactored what arcbench had patched, arcbench's fix re-applied on top; union where both sides added independent functionality. settings.py: union of the env-python-name list (KINBOT_PYTHON and UMA_PYTHON both kept, plus main's GOFLOW_*/RITS_*); ts_adapters keeps arcbench's qst2 and crest alongside main's goflow/rits opt-in note. arc/common.py's recognized-ESS list and ts_adapters_by_rmg_family are likewise unions: arcbench's per-family qst2/crest/ xtb_gsm/orca_neb registrations with main's goflow/rits inserted before 'linear'. set_job_args(): main's narrowed warning (get_dropped_level_args) supersedes arcbench's outright deletion of the old over-firing warning, and Level.get_args() already deep-copies, which was arcbench's other half of the fix. check_negative_freq(): main's (bool, bool) contract, its no-chosen-guess branch and its no-switch-on-unattributable-freqs rule; arcbench's identity-based chosen TS guess lookup is already contained in it. TS guess identity: main's append_ts_guess()/next_ts_guess_index() replace arcbench's manual index assignment at every call site. ARCSpecies.get_next_tsg_index was an exact duplicate of next_ts_guess_index and is removed rather than kept alongside it. Electron counting: main's count_electrons-based get_number_of_electrons and check_multiplicity_parity are kept; arcbench's earlier neutral-count pair, which the merge left as a silently-shadowing second definition of both methods, is removed. Same for the two identical copies of scheduler.tsg_method_matches_adapter and of kinbot_test.kinbot_list_to_coords that the merge duplicated. processor.py: main's rmg_env_command replaces arcbench's hand-rolled launcher ladder, with arcbench's stderr triage and TS-validation reporting kept on top. Kept whole from arcbench: the GCN seeding path (gcn_script/gcn_ts/gcn_test), the seed_hub/FAMILY_SETS heuristics work, run_in_conda_env's extra_env, the ORCA and Gaussian trsh test ladders, the IRC verdict tests, _ts_guess_path_provenance's 'gsm' path slot, _filter_unavailable_ts_adapters, _record_pipe_task_cost and PipeRun.reconcile(scheduler_job_alive=...). Kept whole from main: has_pending_pipe_work, _finalize_species_leaf_task, PipeRun.submit_locally, the zmat coincident-reference-atom refusal, the mapping-engine Rodrigues rotation path, and the goflow/rits adapters. settings_test's default-ts_adapters expectation is updated to the merged default; arc/checks/ts.py is untouched by the merge, so both IRC verdict properties hold (3 n_fragments=0 call sites, 1 authoritative ts_checks['IRC'] = False write). Tests: 3909 passed, 3 failed, 54 skipped. The 3 failures (TestGaussianAdapterGuessMixGating) are pre-existing on arcbench 035cec6 -- their TS0 fixture is a 10-electron composition declared with multiplicity 2.
|
|
||
| if TYPE_CHECKING: | ||
| from arc.molecule.molecule import Atom, Molecule | ||
| from arc.species.species import ARCSpecies |
Check failure
Code scanning / CodeQL
Module-level cyclic import Error
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
To fix this cleanly, remove the TYPE_CHECKING import of ARCSpecies from arc.common so arc.common no longer depends on arc.species.species at all. Since this block is for type hints only, keep type-checker compatibility by defining a local alias ARCSpecies = Any inside the TYPE_CHECKING block. This preserves existing annotations that mention ARCSpecies without creating an inter-module import edge.
Best single change (in arc/common.py, around lines 34–37):
- Keep
AtomandMoleculetype-only import if needed. - Replace
from arc.species.species import ARCSpecieswithARCSpecies = Anyinsideif TYPE_CHECKING:.
No functional/runtime behavior changes are introduced; this only breaks the cyclic dependency for analysis and type resolution purposes.
| @@ -33,7 +33,7 @@ | ||
|
|
||
| if TYPE_CHECKING: | ||
| from arc.molecule.molecule import Atom, Molecule | ||
| from arc.species.species import ARCSpecies | ||
| ARCSpecies = Any | ||
|
|
||
| logger = logging.getLogger('arc') | ||
| logging.getLogger('matplotlib.font_manager').disabled = True |
|
|
||
| from arc.common import ARC_TESTING_PATH, get_logger | ||
| import arc.job.adapters.common as common | ||
| import arc.job.adapters.ts.autotst_ts as autotst_ts |
Check notice
Code scanning / CodeQL
Module is imported with 'import' and 'import from' Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 19 days ago
Use only one import style for arc.job.adapters.ts.autotst_ts in this file. The best minimal fix is:
- Keep
import arc.job.adapters.ts.autotst_ts as autotst_ts(line 16). - Remove
from arc.job.adapters.ts.autotst_ts import AutoTSTAdapter(line 17). - Add a local alias assignment right after imports:
AutoTSTAdapter = autotst_ts.AutoTSTAdapter.
This preserves existing functionality and type hints (-> AutoTSTAdapter) without requiring any other code changes.
Edit only arc/job/adapters/ts/autotst_ts_test.py in the import region (lines 14–19 area).
| @@ -14,11 +14,11 @@ | ||
| from arc.common import ARC_TESTING_PATH, get_logger | ||
| import arc.job.adapters.common as common | ||
| import arc.job.adapters.ts.autotst_ts as autotst_ts | ||
| from arc.job.adapters.ts.autotst_ts import AutoTSTAdapter | ||
| from arc.reaction import ARCReaction | ||
| from arc.species import ARCSpecies | ||
|
|
||
|
|
||
| AutoTSTAdapter = autotst_ts.AutoTSTAdapter | ||
| logger = get_logger() | ||
|
|
||
| TRACEBACK = """Traceback (most recent call last): |
| import os | ||
| import shutil | ||
| import subprocess | ||
| import unittest |
Check notice
Code scanning / CodeQL
Module is imported with 'import' and 'import from' Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 19 days ago
To fix this cleanly without changing behavior, use only one import style for unittest. Since the file already uses unittest.TestCase, unittest.main, and unittest.TextTestRunner, the best minimal fix is:
- Keep
import unittest - Replace
from unittest import mockwithimport unittest.mock as mock
This preserves all existing references to mock while avoiding mixed import styles from the same module.
Make this change in:
arc/job/adapters/ts/autotst_ts_test.py- import section around lines 11–13.
No other code changes are required.
| @@ -9,7 +9,7 @@ | ||
| import shutil | ||
| import subprocess | ||
| import unittest | ||
| from unittest import mock | ||
| import unittest.mock as mock | ||
|
|
||
| from arc.common import ARC_TESTING_PATH, get_logger | ||
| import arc.job.adapters.common as common |
software_release paired a name taken from the level of theory's declared program with a version taken from ess_versions, which is keyed by job type. When a job type had no entry the lookup fell back to the opt job's banner, so a mixed-program run could deposit name='gaussian' with the ORCA banner the opt log carried. The tell was that the mismatch showed up at conformers[0] and never at species level, which is exactly where the job-type lookup misses. The fallback is gone: a job type with no ess_versions entry now yields no version, because another job's banner is not this job's version and TCKDB stores what is deposited as fact. When the record also carries ess_software - the sibling map naming the program that produced each banner - the version is emitted only if that program is the one the level declares, and a disagreement is logged and the version dropped. Records written before ess_software existed carry no such evidence and are unaffected. The banner is also split before it reaches TCKDB. SoftwareReleaseRef keeps version and revision in separate fields and dedupes a release on (software, version, revision, build), so 'Gaussian 16, Revision A.03' is deposited as version='16', revision='A.03' rather than as one string that repeats the program name and forks the release identity. A banner that does not match the recognised shape is passed through whole rather than discarded. Searched for an existing banner splitter before writing one: arc/parser only builds these strings (gaussian.py, orca.py, molpro.py, qchem.py parse_ess_version) and nothing in ARC takes them apart.
Sync the long-lived benchmark integration branch with 75 upstream commits. Conflicts (45 files) were resolved preferring main's reviewed implementation wherever both sides had built the same thing, and keeping arcbench's side only where it is a deliberate divergence main has not received: - arc/imports.py: kept _local_overlays_disabled() (absent on main) alongside main's _report_unusable_overlay(). - arc/job/ssh_pool.py: took main's pool (#1001), a superset of arcbench's prototype, and routed the remaining callers through borrow_ssh_client. - arc/scheduler.py: took main's pooled poll but kept arcbench's stale-server handling, which main does not have. - arc/job/ssh.py: kept arcbench's queue-query tolerance machinery and took main's host-key verification, retrying connect and checkfile deletion. - arc/scripts/save_arkane_thermo.py: kept arcbench's thermo_points, which output.py, ARCSpecies.thermo and the TCKDB adapter consume. - arc/checks/nmd.py: took main's frame/atom-order machinery and kept the ReactionError guard around the bond-change derivation. - arc/settings/settings.py: env-python names stay a union.
| # rotors_dict=None must be preserved — do not re-enable rotor scans. | ||
| self.assertIsNone(sched2.species_dict[ts_label2].rotors_dict) | ||
|
|
||
| def make_irc_scheduler(self, |
Check warning
Code scanning / CodeQL
Variable defined multiple times Warning
| self.assertIsInstance(first[key], (int, float)) | ||
|
|
||
|
|
||
| class TestCommonArgparse(unittest.TestCase): |
Check warning
Code scanning / CodeQL
Variable defined multiple times Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
General fix: ensure each top-level class has a unique name and is defined only once, or remove one duplicate definition if redundant.
Best fix here without changing intended functionality: remove the earlier duplicate TestCommonArgparse block (the one starting around line 150), and keep the later TestCommonArgparse class at line 353. This preserves the canonical class name and avoids any potential downstream references to the later definition changing. Since both blocks test the same parser behavior and the later block is complete, deleting the earlier block resolves the redefinition cleanly.
File/region to change:
arc/scripts_test.py: delete the firstclass TestCommonArgparse(unittest.TestCase):block (lines ~150–170), leaving the second definition untouched.
No new imports, methods, or dependencies are required.
| @@ -147,27 +147,6 @@ | ||
| self.assertIsInstance(first[key], (int, float)) | ||
|
|
||
|
|
||
| class TestCommonArgparse(unittest.TestCase): | ||
| """Test the shared CLI parser used by the standalone scripts.""" | ||
|
|
||
| def test_positional_file_only(self): | ||
| """Without ``--output`` the parser exposes ``args.output is None``.""" | ||
| args = parse_command_line_arguments(['/tmp/in.yml']) | ||
| self.assertEqual(args.file, '/tmp/in.yml') | ||
| self.assertIsNone(args.output) | ||
|
|
||
| def test_output_long_form(self): | ||
| """``--output`` populates ``args.output`` so callers can avoid overwriting input.""" | ||
| args = parse_command_line_arguments(['/tmp/in.yml', '--output', '/tmp/out.yml']) | ||
| self.assertEqual(args.file, '/tmp/in.yml') | ||
| self.assertEqual(args.output, '/tmp/out.yml') | ||
|
|
||
| def test_output_short_form(self): | ||
| """``-o`` is an accepted short form.""" | ||
| args = parse_command_line_arguments(['/tmp/in.yml', '-o', '/tmp/out.yml']) | ||
| self.assertEqual(args.output, '/tmp/out.yml') | ||
|
|
||
|
|
||
| @unittest.skipUnless(RMG_ENV, 'rmg_env not available') | ||
| class TestRmgKineticsHelpers(unittest.TestCase): | ||
| """ |
| patch('arc.scheduler.time.sleep', side_effect=stop_the_loop): | ||
| try: | ||
| self.sched.schedule_jobs() | ||
| except _StopScheduling: |
Check notice
Code scanning / CodeQL
Empty except Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
To fix this without changing functionality, keep catching _StopScheduling but replace the empty pass with an explanatory action (ideally a short comment plus a no-op statement like return in this helper). This preserves the exact control flow while making intent explicit and satisfying the static analysis rule.
In arc/scheduler_test.py, within _run_scheduling_cycle around lines 2696–2699, update:
except _StopScheduling:pass
to:
except _StopScheduling:- explanatory comment indicating this exception is intentionally used to stop the mocked scheduling loop in the test
return end_job(or equivalent non-empty intentional no-op flow)
No imports, new methods, or dependency changes are required.
| @@ -2696,7 +2696,8 @@ | ||
| try: | ||
| self.sched.schedule_jobs() | ||
| except _StopScheduling: | ||
| pass | ||
| # Expected in this test: used as a sentinel to stop the scheduling loop early. | ||
| return end_job | ||
| return end_job | ||
|
|
||
| def test_get_server_job_ids_of_a_failed_query(self): |
| patch('arc.scheduler.time.sleep', side_effect=stop_the_loop): | ||
| try: | ||
| self.sched.schedule_jobs() | ||
| except _StopScheduling: |
Check notice
Code scanning / CodeQL
Empty except Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
The best fix is to keep the current behavior (intentionally swallowing _StopScheduling in tests) while making intent explicit inside the except block. This avoids changing functionality and satisfies the rule by replacing the empty handler with a documented no-op.
In arc/scheduler_test.py, update the except _StopScheduling: block in test_a_failed_query_is_not_offered_to_the_pipe_coordinator (around lines 2737–2740) so it includes an explanatory comment instead of a bare pass. No imports, new methods, or dependency changes are required.
| @@ -2737,6 +2737,8 @@ | ||
| try: | ||
| self.sched.schedule_jobs() | ||
| except _StopScheduling: | ||
| # Expected in this test: used as a control-flow sentinel to break the scheduling loop. | ||
| # Intentionally swallowed so assertions below can verify queue-failure behavior. | ||
| pass | ||
| poll_pipes.assert_called_with(server_job_ids=None) | ||
|
|
|
|
||
| if TYPE_CHECKING: | ||
| from arc.molecule.molecule import Atom, Molecule | ||
| from arc.species.species import ARCSpecies |
Check notice
Code scanning / CodeQL
Unused import Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
The general fix for an unused import is to delete the import statement when the imported symbol is not referenced. For this specific case in arc/common.py, remove from arc.species.species import ARCSpecies from the if TYPE_CHECKING: block and keep the rest unchanged. This preserves runtime behavior (since TYPE_CHECKING is false at runtime anyway) and improves code clarity. No new methods, imports, or definitions are needed.
| @@ -33,7 +33,6 @@ | ||
|
|
||
| if TYPE_CHECKING: | ||
| from arc.molecule.molecule import Atom, Molecule | ||
| from arc.species.species import ARCSpecies | ||
|
|
||
| logger = logging.getLogger('arc') | ||
| logging.getLogger('matplotlib.font_manager').disabled = True |
| # Currently honored by GCN (passed into the ts_gcn subprocess, which seeds python-random, | ||
| # NumPy and PyTorch). CREST 2.12 exposes no seed flag of any kind, so its GFN2-xTB | ||
| # metadynamics cannot be seeded from ARC and remains non-reproducible. | ||
| TS_SEARCH_RANDOM_SEED = 1 |
Check notice
Code scanning / CodeQL
Unused global variable Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
To fix this without changing functionality, explicitly mark TS_SEARCH_RANDOM_SEED as a public settings symbol by adding/updating __all__ in arc/settings/settings.py to include it.
Best single fix:
- In
arc/settings/settings.py, right after definingTS_SEARCH_RANDOM_SEED, add a small__all__update that appends this name. - Use a safe pattern that works whether
__all__is already defined or not:
try:
__all__
except NameError:
__all__ = []
if 'TS_SEARCH_RANDOM_SEED' not in __all__:
__all__.append('TS_SEARCH_RANDOM_SEED')This preserves behavior, keeps the variable name meaningful, and satisfies the query’s “explicitly made public” requirement.
| @@ -340,6 +340,13 @@ | ||
| # metadynamics cannot be seeded from ARC and remains non-reproducible. | ||
| TS_SEARCH_RANDOM_SEED = 1 | ||
|
|
||
| try: | ||
| __all__ | ||
| except NameError: | ||
| __all__ = [] | ||
| if 'TS_SEARCH_RANDOM_SEED' not in __all__: | ||
| __all__.append('TS_SEARCH_RANDOM_SEED') | ||
|
|
||
| # A scan with better resolution (lower number here) takes more time to compute, | ||
| # but the automatically-derived rotor symmetry number is more likely to be correct. | ||
| rotor_scan_resolution = 8.0 # degrees. Default: 8.0 |
Addition of CREST Adapter that complements the heuristic adapter.
This pull request adds support for the CREST conformer and transition state (TS) search method to the ARC project, along with several related improvements and code cleanups. The most important changes include integrating CREST as a TS search adapter, updating configuration and constants, and enhancing the heuristics TS search logic for better provenance tracking and code clarity.
CREST Integration:
JobEnum(arc/job/adapter.py), included CREST in the list of adapters and RMG family mapping, and registered it as a default incore adapter (arc/job/adapters/common.py,arc/job/adapters/ts/__init__.py). [1] [2] [3] [4]arc/job/adapters/ts/crest_test.py).Makefile). [1] [2]Constants and Configuration:
angstrom_to_bohrconversion constant to both Cython and Python constants modules (arc/constants.pxd,arc/constants.py). [1] [2] [3]Heuristics TS Search Enhancements and Refactoring:
arc/job/adapters/ts/heuristics.py). [1] [2] [3] [4]arc/job/adapters/ts/heuristics.py). [1] [2] [3] [4] [5] [6] [7].