#3838 introduce to ket format haptic bond support - #3849
Open
NikolaiBalabanov wants to merge 12 commits into
Open
#3838 introduce to ket format haptic bond support#3849NikolaiBalabanov wants to merge 12 commits into
NikolaiBalabanov wants to merge 12 commits into
Conversation
A molecule node declares the attachment groups whose atoms it holds, and every haptic bond is a connection of the root keyed by `type` rather than by the `connectionType` the monomer connections use, so the haptic branch is taken before that logic. Ids are strings while the member atom list is numbers, as the contract requires; a duplicate id, an unknown group, an out-of-range atom or a value of the wrong JSON type is rejected with a message naming the offender. One connection maps to one HapticBond, in both directions. The pairs an endpoint may not form are left to addHapticBond, which guards them for every producer rather than for this loader alone. Groups are parsed after the merge and added straight to the target molecule through that merge's atom mapping, so a KET group id resolves to a group index without assuming anything about the order in which the merge hands indices out. On saving they are distributed over the nodes after the component split and the connections are written from the whole molecule, because a haptic bond crosses that split: a ring and the metal it binds are separate fragments of the graph. A group whose atoms did not all reach one node is refused outright instead of being written with -1 members. A variable attachment (#3731) shares the container with the other type; KET has no representation for it, so it is dropped rather than written as a haptic bond. A connection without `connectionType` is rejected with a message in the monomer document loader, where a haptic one has no meaning, instead of being read as an absent member.
Six defects, all of the same family: a contract that lived in a comment instead of in the code, so nothing stopped a caller from breaking it. Removing an attachment group is the molecule's business alone - the haptic bonds that address it have to go with it, and the container knows nothing of them. The comment said so; now the method is private with BaseMolecule as its only friend, because the pool hands a freed index straight back and a bond left behind would attach itself to the next group to take that index. addHapticBond is where a group meets the molecule, and it was the one place that could have noticed a member atom nobody owns - the group takes any int. It now does: an index that is not a vertex would otherwise reach the connectivity sets and every consumer of them, down to getAtomXyz in the renderer. SKIP_ALL stopped meaning "skip all" when the two new flags went past 0xFF, so dearomatization and aromatic matching were dragging groups and bonds into the temporary molecules they build. An atom-to-atom bond, meanwhile, was being dropped by SKIP_ATTACHMENT_GROUPS, a flag about groups it does not have. Both merges read the pool as they walked it, so on a merge with itself a copy landing in a freed slot below end() was taken for a source of its own and copied twice. The sources are now listed before anything is added. The decomposition cache recorded "computed with external neighbours" by looking at the first set alone: an s-group without atoms contributes an empty one, and the whole decomposition was then recomputed on every call of the loop in the KET saver. The KET saver kept _mappings, _atom_to_mol_id and _monomers_enum across calls while restarting the node numbering at zero, so a second save on the same instance would have addressed nodes of the first.
The decomposition cache was the sharp one. A haptic bond holds atoms together without an edge, so it is the first thing that changes the answer of countComponents() while the graph, which drops the cache whenever a vertex or an edge moves, sees nothing happen at all: ask once, add or remove a bond, ask again, and the second answer is the first one. Graph now offers invalidateComponents() for exactly this, and the two entry points that edit haptic data call it. The test fails without the call. A template atom stands for a whole monomer and is written as a node of its own, so no format can address it as an end of a haptic bond, nor as a member of a group. Such a molecule used to load and then fail on saving with "Atom %d not found"; addHapticBond now refuses it where the mistake was made. Filling a group went through addAtom(), whose linear search for a duplicate made building one quadratic in the length of the list - and that list comes from a KET file, which the HTTP service takes from anywhere. setAtoms() filters in one pass instead, and the loader hands it the whole list rather than adding atom by atom.
The DoD asks that a ferrocene reactant arrive as one component instead of three. Both connectivity sites were wired in #3837, but nothing exercised the reaction loader, so the guarantee rested on reading the code.
The reader and the writer of a KET field each spelled its name inline, in four files. That is how the highlight round-trip broke: the saver writes "entityType" as "atom"/"bond" and the loader compares against "atoms"/ "bonds", so highlights Indigo writes Indigo does not read. The names the haptic feature touches now come from ket_keys.h; the rest of the KET vocabulary is a migration of its own. Two things the review found on the ids themselves: atoi() read a non-numeric "atomId" as 0 and bound the endpoint to the first atom of the molecule instead of failing, and the stoi() inside extract_id() throws a std::exception rather than an Indigo one on a malformed "moleculeId". Both arrive from outside through the HTTP service. They go through one strict parser now. A duplicate group id was rejected only after the group had been added to the molecule; the check moved ahead of the work. Also drops the attachmentGroupRef() search in saveAttachmentGroups: the KET id of a group is its position in the list that loop already walks.
The group carried the charge and the radical of the star atom it absorbed, so that they would not be lost. The star is no longer absorbed: it stays an ordinary pseudo-atom of the graph, exactly as it is today, and its charge stays on it. There is nothing left for the group to carry. That also settles what the atom-charge sum of a ferrocene is. With the star kept, `ferrocene-variant5.mol` still adds up to zero, the way master reports it now, and the reference output changes only where the feature actually changes something - the connectivity that ENDPTS used to lose. A group is a set of atoms and nothing else.
The comment policy of Task/3233 allows four kinds: an invariant a type cannot express, a pointer to an external source of truth, a trap confirmed by a test, and a short file header. Everything else belongs in the commit message or the Task documents. The keys header broke that worst: thirteen lines defending the decision, all of it already in the previous commit message and in CODE-REVIEW-3838.md, plus section banners restating the constant names. Also drops the ket:: namespace that made every use read ket::KetConnections. The codebase already names KET strings this way in monomers_defs.h (KetConnectionSingle, KetConnectionHydro) with no namespace, and the two awkward ...GroupIdField / ...GroupAtoms names become KetGroupId / KetGroupAtoms. One comment was duplicated verbatim in molecule_attachment_groups.cpp and molecule_haptic_bonds.cpp - two copies of one explanation drift apart, which is exactly what the policy warns about. Both are shortened to a single line.
NikolaiBalabanov
changed the title
The previous pass covered only what this ticket wrote. These two headers came with the model in #3837 and carry the same faults, so they are cleaned here rather than left as the odd ones out. The worst of it was repetition: molecule_attachment_groups.h stated "the haptic bonds address groups by index, so they have to be told" four times - in the class header, above removeGroup, above onAtomsRemoved and above mergeWithSubmolecule. Four copies of one sentence drift apart; it is stated once on the class now, and each method documents only its own contract. The rest is the usual: a class comment defending a design instead of naming an invariant, "Reusable: restore the default-constructed state" restating the method it sits on, and two sentences that could not be parsed on first reading - the ket_document guard and Graph::invalidateComponents. Traps stay untouched, including the one that earns its lines: the group pool recycles freed indices, so a haptic bond outliving its group silently reattaches to whatever takes that index next.
NikolaiBalabanov
requested review from
AlexanderSavelyev and
AliaksandrDziarkach
August 20, 2026 10:22
A reviewer read the constants as class names, and fairly: Ket* is what the classes of ket_objects.h are called - KetConnection, KetConnectionEndPoint, KetMolecule, KetDocument and six more. KetConnections differed from the class KetConnection by one letter. Three things are now visible from a name alone. Keys carry a Field suffix; values do not. monomers_defs.h already holds KetConnectionSingle and KetConnectionHydro, which are values of connectionType, and the old KetConnectionType - a key - was indistinguishable from them. KetConnectionKind and KetConnectionType were the worst pair: kind and type are synonyms, so which one meant "type" and which meant "connectionType" could only be learned by looking. Worse, "type" is not a connection key at all - it labels molecule nodes, rgroups, monomer templates, query components, atom lists and S-group types. It is KetTypeField now, named for what it is. The keys a group object shares with the rest of KET are likewise generic: KetGroupId and KetGroupAtoms claimed a scope they do not have and became KetIdField and KetAtomsField. KetMoleculePrefix is not a key at all, so it says so: KetMoleculeRefPrefix.
The accumulator in parseNumericId was a `long`, so on LLP64 - Win64, the platform this is built and tested on - it is 32 bits and the overflow guard `value > INT_MAX` could never be true. An atomId of "4294967297" wrapped to 1 and bound itself silently to the second atom, which is the exact failure the strict parser was written to prevent. It is a `long long` now, with a test that fails on the old type. The key constants take the KetKey prefix the reviewer suggested. It reads better than the Field suffix it replaces: KetKeyEndpoint1 cannot be confused with the class KetConnectionEndPoint, all keys group together under one prefix, and values keep the bare form of KetConnectionSingle. The suffix `Key` was not an option - monomers_defs.h already spells map lookup keys MonomerKey and NucleotideKey, and those are type aliases. The "connectionType" spelled inside an error message now comes from the constant that the check above it already used. SKIP_ALL moves to the end of the enum and is the OR of every flag rather than a hex literal, so a new flag is added on the line below itself instead of by recounting 0x3FF.
Error messages quoted identifiers with escaped double quotes. The codebase quotes them with single ones - 77 such messages across core, and not one of the other form - so ours were the only ones of their kind, and they reach users through the HTTP service. No test asserts on the quotes, so the change is safe. The fixture returned nothing and one test then reached for atom 10 with a comment saying it was the iron. makeFerrocene returns the metal index now; a magic number explained by a comment is the comment doing a refactoring's job.
The reviewer asked for `long long` because `long` is 32 bits on LLP64 and the
overflow guard could never fire there. That fixes the defect, but it leaves the
arithmetic in place, and the arithmetic is what went wrong: the guard sat after
the multiply, so any 32-bit accumulator wraps before it can be consulted.
`unsigned int` does not help either - it makes the wrap defined rather than
undefined, and "4294967297" still comes back as atom 1; the test built for this
fails on it.
std::from_chars removes the accumulator, so there is no overflow left to reason
about. It is already used in the codebase and already linked into indigo-core
through molecule_cdxml_loader.h, so it reaches every target including WASM.
Checked against the compiler of the CI image, g++ 10.2.1: an out-of-range value
reports result_out_of_range, "12abc" parses as 12 and is caught by ptr != end,
and "-1" parses successfully and is caught by value < 0 - both guards carry
weight rather than decorate.
Neither Scanner::readInt nor extract_id could be reused: the first parses with
sscanf("%d"), which is undefined on overflow, and the second throws a std
exception rather than an Indigo one.
OnlyAPlainDecimalIsAcceptedAsAnAtomId pins what the parser takes, since swapping
a hand-written loop for a library call changes what counts as malformed.
AliaksandrDziarkach
approved these changes
Aug 24, 2026
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
2 participants
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.
Update Ket format loader/saver to support new model including Attachment Group.
Generic request
#1234 – issue name