Fix species copy graph fidelity - #959
Open
calvinp0 wants to merge 2 commits into
Open
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #959 +/- ##
==========================================
- Coverage 65.55% 65.51% -0.05%
==========================================
Files 121 121
Lines 41073 41075 +2
Branches 10566 10566
==========================================
- Hits 26927 26910 -17
- Misses 11115 11133 +18
- Partials 3031 3032 +1
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_species_copy_graph_fidelity
branch
from
August 10, 2026 19:35
b4c5af0 to
e8d7ad7
Compare
calvinp0
force-pushed
the
fix_species_copy_graph_fidelity
branch
from
August 22, 2026 06:44
e8d7ad7 to
4ad7491
Compare
calvinp0
marked this pull request as ready for review
August 22, 2026 08:53
calvinp0
force-pushed
the
fix_species_copy_graph_fidelity
branch
2 times, most recently
from
August 23, 2026 12:13
d698531 to
2eea2bd
Compare
ARCSpecies.copy() round-trips through as_dict()/from_dict(), and from_dict()
called mol_from_xyz() unconditionally whenever the species carried any
coordinates. Perception then replaced the graph it had just deserialized, so
copy() could return a species that is not the one it copied.
Two measured instances. Isoxazole declared as c1ccno1 came back as the
charge-separated [n-]1ccc[o+]1 (net charge 0, so is_mol_valid accepts it);
that is not isomorphic to the neutral form, which mispaired the products in
heuristics.are_h_abs_wells_reversed. Benchmark reaction_018's product P1,
declared [CH]1C=CC2C3=C1C=CC32 (11 C-C bonds, 3 rings), came back as
[CH]1C=C2C=CC3C4C1C234 (12 C-C bonds, 4 rings) because a 1.476 A contact in
the DFT geometry closed a fourth ring; mapping.driver then compared that
re-perceived copy against the live rxn.p_species two lines above it, the bond
counts disagreed, and map_two_species returned None.
Which Lewis structure perception picks is fixed by PYTHONHASHSEED within a
process and is charge-separated for roughly 44% of seeds, so before this
change a copy, a restart, and a pipe-worker rebuild of the same species could
each hold a different molecule.
as_dict() already serializes the graph: rmg_mol_to_dict_repr() stores the
connectivity, bond orders, formal charges, radical counts and the atom order
(sort_atoms=False plus an explicit atom_order), and rmg_mol_from_dict_repr()
restores all of those. It does not restore atom labels - the repr carries a
'label' per atom but the deserializer constructs each Atom without one, so RMG
family site labels ('*1', '*2', '*3') come back empty. That is not a regression,
since perception dropped them too, and mapping.engine already copies them back
by hand; restoring them is a separate change. Labels aside, the mapping is only
ever written by as_dict(), from a live object whose graph and coordinates
already stood in whatever relation they stood in, so re-perceiving it can only
diverge from the object being reproduced. dict_repr_is_authoritative() names
that condition and from_dict() now skips perception when it holds.
The predicate deliberately excludes every user-authored structure - smiles,
inchi, adjlist, and an adjacency list stored under the 'mol' key - because
their atom order bears no relation to the coordinates, and rotor indices and
atom maps index the graph against the coordinates. Those keep re-perceiving,
as does a dict carrying coordinates and no graph at all. ARCSpecies(smiles=,
xyz=) is likewise left alone for the same reason; keep_mol=True remains its
opt-out, as mapping.engine.copy_species_list_for_mapping already uses it.
from_yml_file() has expressed the same idea for longer through its regen_mol
flag - a 'mol' dict repr there supersedes perception, with no type or
uniqueness guard at all - and is the obvious target for consolidation, which
this change does not attempt.
It also excludes a mapping whose atom IDs are not unique, which the decoded
graph reports through the existing Molecule.atom_ids_valid(). rmg_mol_from_dict_repr()
keys both the atoms and their edges by ID, so a mapping with duplicate IDs
decodes to fewer distinct atoms than it has atom slots; a hand-written fixture
in species_test.test_from_dict decodes to ten references to a single atom, and
its garbage graph was previously masked by the perception this change removes.
rmg_mol_to_dict_repr() can emit such a mapping as well: it only compares the IDs
of the first two atoms before deciding to reassign, so a collision anywhere
later is serialized intact (Molecule(smiles='CCO') with assigned IDs and the
last atom's ID reset to the first's re-decodes to nine slots over eight atoms).
Nothing in ARC produces one today. On such a dict the predicate is False and
from_dict() falls back to the pre-fix perception behaviour rather than trusting
an undecodable graph. Giving rmg_mol_to_dict_repr() the same atom_ids_valid()
test is separable hardening, not done here.
The mirror-image bug in ARCSpecies.__init__ is untouched:
ARCReaction.get_single_mapped_product_xyz() hands __init__ a mol together with
an xyz and no keep_mol=True, so __init__ clobbers the graph it was just given.
Both new copy tests inject the perception result rather than relying on a hash
seed, since an end-to-end perception test is flaky in both directions, and they
assert that perception is not called at all. The ring closure they inject
stands in for reaction_018's measured 1.476 A contact and is not a claim about
butadiene, whose s-trans geometry has no such contact. The two from_dict tests
are controls: they fail if perception is suppressed too widely.
The species-entry reference described 'mol' only as an RMG Molecule object for the Python API and did not say what happens when a dict lands there. ARC.py turns any mapping in the input file's species list into ARCSpecies(species_dict=...), so a 'mol' block copied out of a restart file now supersedes the coordinates instead of being replaced by a graph perceived from them, and coordinates that disagree with it no longer raise at load time. 'keep_mol' gets the same note, since it is no longer needed for that case.
calvinp0
force-pushed
the
fix_species_copy_graph_fidelity
branch
from
September 7, 2026 17:42
2eea2bd to
ae42dcd
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.
This pull request introduces a new mechanism for handling species graph perception from input dictionaries, ensuring that serialized 2D molecular graphs are preserved when explicitly provided. This prevents unwanted changes to molecular connectivity or resonance structures during restarts or copy operations. The changes include a new utility function, updates to the main logic in
from_dict, comprehensive tests, and improved documentation.Core logic improvements:
dict_repr_is_authoritativefunction to determine if a species dictionary'smolkey contains a serialized graph that should be used as-is, rather than re-perceiving the structure from coordinates.from_dictmethod inARCSpeciesto usedict_repr_is_authoritative, so that a provided serialized graph is preserved and not replaced by a graph perceived from coordinates. [1] [2]Testing enhancements:
dict_repr_is_authoritativeand for copy/restart behavior, ensuring that declared molecular graphs are preserved and not overridden by coordinate perception. [1] [2] [3]Documentation updates:
input_reference.rstto clearly document the new behavior: when a serializedmolgraph is present in a species dictionary, it is always used as-is, and thekeep_molflag is not needed for such cases. [1] [2]Test infrastructure:
mockin the test module to enable patching and testing of perception logic.