Skip to content

Commit 360de8c

Browse files
stsoukoclaude
andcommitted
3.0.2: place leaving and arriving atoms in the transition-state view
The union was keyed by map number, so an atom present on one side only had either no key or no partner and was left out, taking its bonds with it. `smiles('N1CC1') @ smiles('c1ccccc1Br')` reported the aryl carbon as 3 neighbours before and 3 after with the C-Br bond absent -- a substitution that reads as no change at all. The union is keyed on atoms. An atom on one side only holds a row on the side it came from, and on the far side counts only the neighbours that left or arrived with it: its attachment atom is decremented by a leaving group's bonds to the retained part and incremented by an installed one's, and a byproduct still reads as the molecule it is. Being on one side only is not the same thing as being unnumbered -- whether the record numbered it changes its key and nothing else, so the element, hydrogen, degree and bond columns are identical for [CH3:1][Br:2]>>[CH4:1] and [CH3:1]Br>>[CH4:1]. `modeling_view()` keys an unnumbered atom -1 onwards in union order, `unmapped` still counting it. The reactor, `mol_state_view` and `mol_transition_view` are untouched, so the neural mapper -- which reads `state_view` -- is unaffected, and a molecule's transition view still emits both halves equal: molecules and reactions keep one vocabulary. docs/ml.rst gains "What a record can state", where a leaving atom, an arriving atom, a hydrogen count change, a degree change and a bond order change each land, and the tests cover those five and their combinations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent d5b04fb commit 360de8c

9 files changed

Lines changed: 493 additions & 115 deletions

File tree

‎chython/core/_ml.pxi‎

Lines changed: 77 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -683,23 +683,41 @@ def mol_transition_view(MoleculeContainer molecule not None, TensorEncoding enco
683683
DEF ML_NO_INDEX = 0xffffffff
684684

685685

686-
# `uidx` is `map_number -> union index`, valid only for touched slots. `rseen` / `pseen` mark which
687-
# side already claimed a map number. `keep` is indexed by running ordinal over all atoms of both
688-
# sides (left first, then right); the atom passes write 1 for an atom that took a union slot and 0 for
689-
# one that was unmapped or rejected; the bond passes read it for both endpoints. `head` / `link` are a
690-
# per-atom singly linked list over bond slots, keyed on the lower union index of the pair. `side`
691-
# codes: 1 = reactant-only, 2 = product-only, 3 = both. The uidx table is max map number + 1, at most
692-
# 10000 slots -- initialized slot by slot, not memset, because a three-atom record must not pay for all.
686+
# `uidx` is `map_number -> union index`, valid only for touched slots, and it is what PAIRS the two
687+
# sides -- an unmapped atom therefore takes a union row without an entry in it. `rseen` / `pseen` mark
688+
# which side already claimed a map number. `urow` is indexed by running ordinal over all atoms of both
689+
# sides (left first, then right) and holds the union row the atom took, or `ML_NO_INDEX` for a rejected
690+
# collision; the bond passes read it for both endpoints, which is why they need no map number of their
691+
# own. `head` / `link` are a per-atom singly linked list over bond slots, keyed on the lower union index
692+
# of the pair. `side` codes: 1 = reactant-only, 2 = product-only, 3 = both. The uidx table is max map
693+
# number + 1, at most 10000 slots -- initialized slot by slot, not memset, because a three-atom record
694+
# must not pay for all.
693695
cdef struct ml_merge_t:
694696
uint32_t *uidx
695697
uint8_t *rseen
696698
uint8_t *pseen
697699
uint8_t *side # 1 = reactant-only, 2 = product-only, 3 = both
698-
uint8_t *keep # 1 = atom took a union slot, 0 = unmapped or collided
700+
uint32_t *urow # union row per atom ordinal; ML_NO_INDEX = a rejected collision
699701
uint32_t *head
700702
uint32_t *link
701703

702704

705+
cdef inline void _ml_union_row(ml_union_t *g, ml_merge_t *merge, uint32_t row, atom_t *at,
706+
uint32_t mn, uint8_t side) noexcept nogil:
707+
"""One union row from one atom, on the side that atom came from.
708+
709+
`h_after` is written equal to `h_before`, which a paired product atom then overwrites. An atom on
710+
one side only keeps it -- the convention `ReactionModelingView.states` states -- and an unmapped
711+
atom is one of those, the record having said nothing that would pair it.
712+
"""
713+
merge.side[row] = side
714+
merge.head[row] = ML_NO_INDEX
715+
g.element[row] = at.element
716+
g.h_before[row] = at.hydrogens & 0x0f
717+
g.h_after[row] = at.hydrogens & 0x0f
718+
g.map_number[row] = <uint16_t> mn
719+
720+
703721
cdef void _ml_union_degrees(ml_union_t *union_graph, ml_merge_t *merge) noexcept nogil:
704722
"""Per-side heavy degree over the union, by the rule `ReactionModelingView.states` states.
705723
@@ -744,9 +762,16 @@ def reaction_transition_view(reactants, products, TensorEncoding encoding=None):
744762
AGENTS ARE EXCLUDED BY NOT BEING PASSED. The caller hands over the two sides it wants unioned, so
745763
there is no side-kind test inside the kernel to keep in step with the container's notion of one.
746764
747-
A BADLY MAPPED RECORD IS REPORTED. An unmapped atom is counted and left out; a map number claimed
748-
twice on one side is listed in `collisions` and the first claim keeps the slot. Refusals live at
749-
the answer boundary and this is not one.
765+
AN UNMAPPED ATOM IS PLACED ON THE SIDE IT CAME FROM, as reactant-only or product-only, and counted
766+
in `unmapped`. Left out, it takes the degree of every neighbour that stayed down with it: an aryl
767+
bromide whose bromine carries no number reads two heavy neighbours before the coupling and three
768+
after, so every aryl halide of one ring gives the same transition state. The price is that an atom
769+
the record leaves bare on BOTH sides is two rows rather than one, nothing in the record pairing
770+
them, and `unmapped` is what a consumer that cannot accept that reads.
771+
772+
A BADLY MAPPED RECORD IS REPORTED. A map number claimed twice on one side is listed in `collisions`
773+
and the first claim keeps the row; the second contributes nothing, since a union cannot hold two
774+
atoms at one key. Refusals live at the answer boundary and this is not one.
750775
"""
751776
require_numpy()
752777
if encoding is None:
@@ -787,7 +812,7 @@ def reaction_transition_view(reactants, products, TensorEncoding encoding=None):
787812
+ align8(<size_t> slots) # rseen
788813
+ align8(<size_t> slots) # pseen
789814
+ align8(<size_t> cap_atoms) # side
790-
+ align8(<size_t> cap_atoms) # keep
815+
+ align8(<size_t> cap_atoms * sizeof(uint32_t)) # urow
791816
+ align8(<size_t> cap_atoms * sizeof(uint32_t)) # head
792817
+ align8(<size_t> cap_bonds * sizeof(uint32_t)) # link
793818
+ <size_t> 4 * cap_atoms # element + h_before + n_before + h_after
@@ -806,7 +831,7 @@ def reaction_transition_view(reactants, products, TensorEncoding encoding=None):
806831
merge.rseen = <uint8_t *> cursor; cursor += align8(<size_t> slots)
807832
merge.pseen = <uint8_t *> cursor; cursor += align8(<size_t> slots)
808833
merge.side = <uint8_t *> cursor; cursor += align8(<size_t> cap_atoms)
809-
merge.keep = <uint8_t *> cursor; cursor += align8(<size_t> cap_atoms)
834+
merge.urow = <uint32_t *> cursor; cursor += align8(<size_t> cap_atoms * sizeof(uint32_t))
810835
merge.head = <uint32_t *> cursor; cursor += align8(<size_t> cap_atoms * sizeof(uint32_t))
811836
merge.link = <uint32_t *> cursor; cursor += align8(<size_t> cap_bonds * sizeof(uint32_t))
812837
union_graph.element = <uint8_t *> cursor; cursor += cap_atoms
@@ -826,7 +851,7 @@ def reaction_transition_view(reactants, products, TensorEncoding encoding=None):
826851
cdef uint32_t n_union = 0, n_bonds = 0
827852
cdef uint32_t unmapped_r = 0, unmapped_p = 0
828853
cdef list collisions_r = [], collisions_p = []
829-
cdef uint32_t k, mn, mm, n, m, e, base, right_base
854+
cdef uint32_t k, mn, n, m, e, base, right_base
830855
cdef uint32_t edge_to
831856
cdef uint8_t order
832857
try:
@@ -853,26 +878,20 @@ def reaction_transition_view(reactants, products, TensorEncoding encoding=None):
853878
for i in range(n_mol):
854879
at = atoms + i
855880
mn = at.map_number
856-
if not mn:
881+
if mn:
882+
if merge.rseen[mn]:
883+
collisions_r.append(mn)
884+
merge.urow[base + i] = ML_NO_INDEX
885+
continue
886+
merge.rseen[mn] = 1
887+
merge.uidx[mn] = n_union
888+
else:
857889
unmapped_r += 1
858-
merge.keep[base + i] = 0
859-
continue
860-
if merge.rseen[mn]:
861-
collisions_r.append(mn)
862-
merge.keep[base + i] = 0
863-
continue
864-
merge.rseen[mn] = 1
865-
merge.uidx[mn] = n_union
866-
merge.side[n_union] = 1
867-
merge.head[n_union] = ML_NO_INDEX
868-
merge.keep[base + i] = 1
869-
union_graph.element[n_union] = at.element
870-
union_graph.h_before[n_union] = at.hydrogens & 0x0f
871-
union_graph.h_after[n_union] = at.hydrogens & 0x0f
872-
union_graph.map_number[n_union] = <uint16_t> mn
890+
_ml_union_row(&union_graph, &merge, n_union, at, mn, 1)
891+
merge.urow[base + i] = n_union
873892
n_union += 1
874893
base += n_mol
875-
right_base = base # starting ordinal for product atoms in the keep array
894+
right_base = base # starting ordinal for product atoms in the urow array
876895

877896
# product atoms: a known map number reads its after state onto the existing row, a new one
878897
# appends -- which is what keeps a component contiguous in the union order
@@ -883,54 +902,42 @@ def reaction_transition_view(reactants, products, TensorEncoding encoding=None):
883902
for i in range(n_mol):
884903
at = atoms + i
885904
mn = at.map_number
886-
if not mn:
887-
unmapped_p += 1
888-
merge.keep[base + i] = 0
889-
continue
890-
if merge.pseen[mn]:
891-
collisions_p.append(mn)
892-
merge.keep[base + i] = 0
893-
continue
894-
merge.pseen[mn] = 1
895-
if merge.rseen[mn]:
896-
n = merge.uidx[mn]
897-
merge.side[n] = 3
898-
union_graph.h_after[n] = at.hydrogens & 0x0f
899-
merge.keep[base + i] = 1
900-
else:
905+
if mn:
906+
if merge.pseen[mn]:
907+
collisions_p.append(mn)
908+
merge.urow[base + i] = ML_NO_INDEX
909+
continue
910+
merge.pseen[mn] = 1
911+
if merge.rseen[mn]:
912+
n = merge.uidx[mn]
913+
merge.side[n] = 3
914+
union_graph.h_after[n] = at.hydrogens & 0x0f
915+
merge.urow[base + i] = n
916+
continue
901917
merge.uidx[mn] = n_union
902-
merge.side[n_union] = 2
903-
merge.head[n_union] = ML_NO_INDEX
904-
merge.keep[base + i] = 1
905-
union_graph.element[n_union] = at.element
906-
union_graph.h_before[n_union] = at.hydrogens & 0x0f
907-
union_graph.h_after[n_union] = at.hydrogens & 0x0f
908-
union_graph.map_number[n_union] = <uint16_t> mn
909-
n_union += 1
918+
else:
919+
unmapped_p += 1
920+
_ml_union_row(&union_graph, &merge, n_union, at, mn, 2)
921+
merge.urow[base + i] = n_union
922+
n_union += 1
910923
base += n_mol
911924

912-
# reactant bonds, both endpoints mapped and kept: appended in reactant order, `n < m`.
913-
# The `n >= m` guard also stops a bond between two atoms sharing one map number from
914-
# becoming a self-loop -- that is load-bearing, not incidental.
925+
# reactant bonds, both endpoints holding a union row: appended in reactant order, `n < m`, so
926+
# the `n >= m` guard is what visits each bond once.
915927
base = 0
916928
for mol in left:
917929
structure = mol._structure
918-
atoms = structure.atoms()
919930
ptr = csr_ptr(structure)
920931
edges = csr_edges(structure)
921932
n_mol = structure.header.atom_count
922933
for i in range(n_mol):
923-
if not merge.keep[base + i]:
934+
n = merge.urow[base + i]
935+
if n == ML_NO_INDEX:
924936
continue
925-
mn = atoms[i].map_number
926-
n = merge.uidx[mn]
927937
for k in range(ptr[i], ptr[i + 1]):
928938
edge_to = edges[k].to
929-
if not merge.keep[base + edge_to]:
930-
continue
931-
mm = atoms[edge_to].map_number
932-
m = merge.uidx[mm]
933-
if n >= m:
939+
m = merge.urow[base + edge_to]
940+
if m == ML_NO_INDEX or n >= m:
934941
continue
935942
union_graph.bond_i[n_bonds] = n
936943
union_graph.bond_j[n_bonds] = m
@@ -945,22 +952,17 @@ def reaction_transition_view(reactants, products, TensorEncoding encoding=None):
945952
base = right_base
946953
for mol in right:
947954
structure = mol._structure
948-
atoms = structure.atoms()
949955
ptr = csr_ptr(structure)
950956
edges = csr_edges(structure)
951957
n_mol = structure.header.atom_count
952958
for i in range(n_mol):
953-
if not merge.keep[base + i]:
959+
n = merge.urow[base + i]
960+
if n == ML_NO_INDEX:
954961
continue
955-
mn = atoms[i].map_number
956-
n = merge.uidx[mn]
957962
for k in range(ptr[i], ptr[i + 1]):
958963
edge_to = edges[k].to
959-
if not merge.keep[base + edge_to]:
960-
continue
961-
mm = atoms[edge_to].map_number
962-
m = merge.uidx[mm]
963-
if n >= m:
964+
m = merge.urow[base + edge_to]
965+
if m == ML_NO_INDEX or n >= m:
964966
continue
965967
order = edges[k].order
966968
e = merge.head[n]

‎chython/core/reaction.py‎

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -202,8 +202,8 @@ class ReactionModelingView:
202202
number is the only thing that identifies "the same atom" across the arrow -- keying by atom number
203203
works only where a reader makes the two coincide.
204204
205-
IT REPORTS WHAT IT COULD NOT PLACE instead of producing an undefined union. Calling unmapped
206-
atoms and colliding map numbers "undefined results" means, for a training set, silent corruption at
205+
IT REPORTS WHAT THE RECORD DID NOT STATE instead of leaving it undefined. Calling unmapped atoms
206+
and colliding map numbers "undefined results" means, for a training set, silent corruption at
207207
whatever rate the corpus happens to contain. Here they land in :attr:`unmapped` and
208208
:attr:`collisions`, both empty for a well-mapped reaction, so a pipeline can decide -- and a
209209
refusal, if one is wanted, happens at the pipeline's boundary rather than inside a container that
@@ -219,7 +219,11 @@ def __init__(self, states, union_bonds, unmapped, collisions):
219219

220220
@property
221221
def states(self) -> dict[int, tuple]:
222-
"""`{map_number: (element, h_before, n_before, h_after, n_after)}`.
222+
"""`{map_number: (element, h_before, n_before, h_after, n_after)}`, in union order.
223+
224+
A NEGATIVE KEY IS AN ATOM THE RECORD DOES NOT NUMBER, allocated `-1` onwards in that order. It
225+
is placed like any other atom, as reactant-only or product-only; the negative key says the
226+
pairing is this view's reading of an absence rather than something the record stated.
223227
224228
`element` is the atomic number. `h_*` are IMPLICIT hydrogen counts per side and `n_*` are
225229
heavy-atom neighbour counts per side -- the per-side hydrogen count being the thing the
@@ -253,9 +257,11 @@ def union_bonds(self) -> dict[tuple[int, int], tuple[int, int]]:
253257
def unmapped(self) -> dict[str, int]:
254258
"""How many atoms carried no map number, per side: `{'reactants': n, 'products': n}`.
255259
256-
Not an error and not silently dropped. An unmapped atom cannot be placed in a union keyed by
257-
map number, so it is counted and left out, and a consumer that requires a fully mapped
258-
reaction checks this rather than discovering the hole in its loss curve.
260+
Not an error and not a refusal. Each is placed on the side it came from and keyed negatively in
261+
:attr:`states`, so no neighbour of one loses a degree; what the count is for is the atom the
262+
record leaves bare on BOTH sides, which becomes two rows because nothing pairs them. A consumer
263+
that requires a fully mapped reaction checks this rather than discovering the hole in its loss
264+
curve.
259265
"""
260266
return self._unmapped
261267

@@ -577,18 +583,28 @@ def modeling_view(self) -> ReactionModelingView:
577583
that is also a drawing. Stated rather than assumed, because it is the kind of choice a model
578584
silently trains around.
579585
586+
AN UNMAPPED ATOM IS KEYED NEGATIVELY, `-1` onwards in union order, and still counted in
587+
`unmapped`. It holds a union row like any other atom -- on the side it came from -- so the key
588+
is only the identity handle a dict needs: 0 would put every such atom on one entry, and a
589+
positive one would be indistinguishable from a number the record stated.
590+
580591
A COLLIDING MAP NUMBER KEEPS ITS FIRST CLAIM. The second atom to claim a number is counted in
581592
`collisions` and contributes no state and no bond; a union cannot hold two atoms at one key.
582593
"""
583594
view = reaction_transition_view(self._reactants, self._products,
584595
TensorEncoding(unknown_h=H_UNKNOWN))
585-
map_numbers = view.map_numbers.tolist()
596+
keys, bare = [], 0
597+
for mn in view.map_numbers.tolist():
598+
if not mn:
599+
bare -= 1
600+
mn = bare
601+
keys.append(mn)
586602
states = {mn: (int(view.elements[i]), int(view.h_before[i]), int(view.n_before[i]),
587603
int(view.h_after[i]), int(view.n_after[i]))
588-
for i, mn in enumerate(map_numbers)}
604+
for i, mn in enumerate(keys)}
589605
union = {}
590606
for (i, j), before, after in zip(view.bonds.tolist(), view.bond_before, view.bond_after):
591-
a, b = map_numbers[i], map_numbers[j]
607+
a, b = keys[i], keys[j]
592608
union[(a, b) if a < b else (b, a)] = (int(before), int(after))
593609
return ReactionModelingView(states, union, view.unmapped, view.collisions)
594610

‎chython/core/test/modeling_view_corpus.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@
6060
# the mirror: 5..8 exist only on the product side
6161
'tert_butyl_esterification': '[CH3:1][C:2](=[O:3])[OH:9]'
6262
'>>[CH3:1][C:2](=[O:3])[O:4][C:5]([CH3:6])([CH3:7])[CH3:8].[OH2:9]',
63-
# one unmapped atom on each side, counted and left out of the union
63+
# two unmapped atoms on each side, counted and placed on the side they came from
6464
'esterification_partly_mapped': '[CH3:1][C:2](=[O:3])[OH:4].CO'
6565
'>>[CH3:1][C:2](=[O:3])[O:4]C.O',
6666
# two atoms sharing map number 1 on the reactant side: the union merges them and says so
@@ -74,7 +74,7 @@
7474
# a salt on the product side: two molecules, one of them a single mapped ion
7575
'saponification': '[CH3:1][C:2](=[O:3])[O:4][CH3:5].[OH-:6].[Na+:7]'
7676
'>>[CH3:1][C:2](=[O:3])[O-:4].[Na+:7].[CH3:5][OH:6]',
77-
# no mapping at all: every atom is unmapped, the union is empty, and that is a record
77+
# no mapping at all: every atom is on one side only, so nothing is conserved, and that is a record
7878
'unmapped_hydrogenation': 'C=C.[H][H]>>CC',
7979
# an empty product side, which is a record and not an error
8080
'empty_products': '[CH3:1][CH3:2]>>',

0 commit comments

Comments
 (0)