Stop the charge filtration heuristics keying on sorting_label - #1011
Open
calvinp0 wants to merge 4 commits into
Open
Stop the charge filtration heuristics keying on sorting_label#1011calvinp0 wants to merge 4 commits into
calvinp0 wants to merge 4 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1011 +/- ##
==========================================
- Coverage 64.60% 64.57% -0.04%
==========================================
Files 119 119
Lines 39785 39796 +11
Branches 10307 10306 -1
==========================================
- Hits 25703 25698 -5
- Misses 11105 11107 +2
- Partials 2977 2991 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
calvinp0
force-pushed
the
fix_resonance_save_order
branch
from
August 23, 2026 08:56
eb397c7 to
7ea20bc
Compare
_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.
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.
calvinp0
force-pushed
the
fix_resonance_save_order
branch
from
August 23, 2026 12:14
7ea20bc to
c31521e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resonance generation returns different structures depending on
save_order, and for some species depending on the process it runs in. Both come from the charge filtration heuristics identifying atoms bysorting_label.The defect
charge_filtration,find_unique_sites_in_charged_listandstabilize_charges_by_proximitykey onatom.sorting_label, which the isomorphism machinery leaves either unset or holding a stale permutation that no longer matches the vertex order.Under
save_orderthe structures of one species arrive in a mixed state. For 4-nitrophenoxy, 12 of the 18 structures reachingcharge_filtrationcarry the unset value and the other 6 a stale permutation — against 18 of 18 correctly labelled withoutsave_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 like 4-nitrophenoxy that removed every aromatic structure.The fix
Identify atoms by their position in
mol.verticesvia a newget_atom_indices, and replace the label comparison instabilize_charges_by_proximitywithitertools.combinations, guarding the disconnected case wherefind_shortest_pathreturnsNone.save_orderis also forwarded to the dispatched resonance generation methods.Measured
Over a 189-species corpus, the number of species whose distinct structures differ between the two
save_ordersettings goes from 10 to 0. It also removes a run-to-run nondeterminism: 4-nitrophenoxy undersave_order=Truereturned 9 or 7 structures depending on the process, and now returns 11 in every run — matchingsave_order=False. No structure is lost anywhere in the corpus, under either setting.It restores the three worked examples in
filtration.py's own module docstring, all of whichsave_order=Truecontradicted:Two corrections needed to keep that true
A precedence bug 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 too — N#S recorded[(0, 1), (1, 0)].find_unique_sites_in_charged_listis the only reader ofmul_bond_sorting_listand looks up ordered pairs only, so the reversed entries were never queried and this changes no results; it is corrected because the index change would otherwise make it reachable.Disconnected fragments. Salts whose charged atoms sit in different molecular fragments raised
TypeErrorfrom the charge proximity heuristic, or were filtered away entirely.Verification
82 passedacrossarc/molecule/resonance_test.pyandarc/molecule/filtration_test.py, with the extensions rebuilt — bothresonance.pyandfiltration.pyare Cythonised, so a stale.sowould silently test the wrong code.New tests cover the nitroaromatic radicals whose aromatic structures were dropped, the NO₂ / CH₂NO / NH₂CHO docstring examples asserted for both
save_ordersettings, the disconnected-fragment salts, and a unit test forget_atom_indices.Rebased from 94 behind onto current
main; the changed-line set is identical in both directions against its old merge-base, so nothing was dropped. Four commits, four files, each file in exactly one commit.🤖 Generated with Claude Code