Skip to content

Improve RDKit descriptor parity and extension CI - #14

Open
nkwork9999 wants to merge 9 commits into
mainfrom
feature/rdkit-parity
Open

Improve RDKit descriptor parity and extension CI#14
nkwork9999 wants to merge 9 commits into
mainfrom
feature/rdkit-parity

Conversation

@nkwork9999

@nkwork9999 nkwork9999 commented Aug 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

CI verified (2026-09-05)

Verified source commit: ff7c681ca14698d7ed6ec4a5d7a15fc40cefe834. Community PR head: 42d25c854f8720d561ace44082c7642e09e0c46d (tested merge commit 65f5631b4c72dbf66b1eb49c5eb62c191c1dbca3).

Existing disabled/documentation/deployment jobs remain skipped according to the workflows; they are not counted as passed. No platform exclusions or test-disabling changes were added. These results verify builds and the executed tests, not publication or universal RDKit compatibility. Wasm checks establish side-module link integrity, not DuckDB-Wasm runtime SQL coverage.

Build and runtime compatibility fixes

  • Keep DuckDB 1.5.5 and development vector APIs compatible, including nullable callbacks, mutable output access and read-only list children. Add constant/NULL, filtered-vector and multi-batch regressions.
  • Mark the ten BLOB similarity callbacks and 25 dynamic-string callbacks that can throw as fallible; test expected errors, row-wise TRY recovery and connection reuse.
  • Correct cross-platform Rust target selection and toolchain declarations; forward all Rust archives into the actual Wasm final link.
  • Match pinned Emscripten 3.1.71 with Rust nightly-2025-02-01 and a Wasm-only std rebuild, including threaded features. Validate missing symbols and self-resolved GOT relocations against the actual binary. Wasm build contract.

Current revision (ff7c681)

This PR improves measured RDKit parity; it does not establish equivalence for every molecule, parser input, sanitization result, or fingerprint.

  • Adds a separate suite for the 21 structural-profile descriptors, pinned to RDKit 2025.09.6 with explicit input hydrogens retained.
  • 4,510 distinct SMILES inputs × 21 outputs = 94,710 comparisons. Inputs include equivalent spellings, isolated elements and isotopes, not 4,510 independent drug-like molecules. Integers match exactly; floating values use an absolute tolerance of 1e-9.
  • Fixes isotope-aware heavy-atom mass, hydrogen partitioning, aromatic macrocycles, fused-system aromatic bonds and selected radical cases.
  • Local workspace result: 676 passed, 1 existing ignored. Latest push and PR Rust test workflows pass.
  • Uses DuckDB v1.5.5 / v1.5-variegata tools, requests Rust, fixes stable/new ListVector header compatibility and adds tested cross-target selection. Latest format, tidy, binary-build and executed SQL-test jobs pass; see the verified CI links above.
  • No RDKit runtime dependency was added. Universal compatibility remains unresolved and is not claimed by this finite reference suite.

Definitions, reproduction and limits: PROFILE_PARITY.md.

Community update: duckdb/community-extensions#2055

Earlier implementation notes

The original implementation was cross-checked against RDKit 2026.03.5 over 214 molecules and 1,188 equivalent spellings. The sections below describe that earlier work; current additional validation and CI status are summarized above.

What was wrong

Area Defect
Aromaticity Fused rings written in Kekulé form were misperceived (Kekulé naphthalene reported one aromatic ring). 10 of 30 molecules answered differently depending on notation.
Canonical SMILES Not canonical: 16 of 45 molecules produced different output for different atom orderings. Stereocentres inverted when the output order changed.
Ring perception Bridged polycycles came up short of RDKit's symmetrised SSSR.
H-bond acceptors Matched none of RDKit's definitions (27% mismatch).
Murcko scaffolds Exocyclic double-bonded atoms dropped; some outputs were not valid SMILES.
Crippen logP Aromatic N-H hydrogen typed as H2 instead of H3.
Rotatable bonds Ester and trifluoromethyl bonds over-counted.
SMARTS R<n> treated as "in a ring"; dot-separated patterns unsupported, disabling two QED alerts.
String FFI ds_canonical_smiles returned -1 on a small buffer; ds_mol_formula and ds_add_hydrogens truncated silently.
Morgan Not spelling-invariant; pairwise Tanimoto tracked RDKit at rho = 0.845.

What changed

  • Aromaticity — Hückel 4n+2 electron count per smallest ring and per fused system, judged against a snapshot of the original bond orders. Implicit hydrogens are now filled in before perception.
  • Canonicalisation — canonical_ranks refines graph-only atom invariants with deterministic tie-breaking; the writer orders neighbours by rank, recomputes tetrahedral parity against the emitted order, and normalises cis/trans marks into a canonical frame.
  • Rings — keep the relevant cycles, those not expressible as a GF(2) sum of strictly shorter cycles. Bicyclo[2.2.2]octane has three rings, adamantane four.
  • Definitions — acceptors, donors and rotatable bonds now follow RDKit's own SMARTS exactly.
  • Scaffolds — exocyclic double-bonded atoms retained, severed bonds replaced by hydrogens, stereo dropped only when the neighbour set changed.
  • FFI — all string functions follow the sizing protocol, and the fixed-size C++ buffers retry on the heap.
  • Morgan — shared ring perception, canonical dedup order, and an avalanche step before folding. rho > 0.95.

Tests

scripts/gen_rdkit_fixtures.py regenerates every fixture. 47 integration tests cover descriptor parity, the canonical-form contract (order invariance, idempotence, round-trip, distinctness, stereo), Kekulé/aromatic equivalence, scaffold rules, fingerprints and buffer contracts.

Canonical and scaffold outputs are compared structurally rather than byte-wise: two canonicalisers may legitimately choose different spellings of the same molecule, so RDKit's spelling is fed back through ds_canonical_smiles and required to land on ours.

546 unit tests plus 47 integration tests pass, with zero compiler warnings.

🤖 Generated with Claude Code

nkwork9999 and others added 5 commits August 7, 2026 22:51
@nkwork9999 @claude
Reach RDKit parity across every scalar descriptor
70e17ea 
Cross-checking duckSMILES against RDKit 2026.03.5 over 214 molecules
turned up defects in nearly every layer of the library. This fixes all of
them and adds the parity suite that pins the behaviour down.

Aromaticity
- Replace the 6-ring single/double counting with a Hueckel 4n+2 electron
  count over each smallest ring, then over each fused ring system. Every
  ring is judged against a snapshot of the original bond orders, so a
  fused neighbour that was rewritten first can no longer change the
  answer -- Kekule naphthalene used to report one aromatic ring.
- Fill implicit hydrogens before perceiving aromaticity, otherwise a
  Kekule pyrrole nitrogen loses its hydrogen to the aromatic valence.

Canonical SMILES
- Add canonical_ranks: refinement of graph-only atom invariants with
  deterministic tie-breaking. The writer now orders neighbours by rank,
  so re-spelling a molecule cannot change the output. 16 of 45 test
  molecules used to canonicalise differently depending on atom order.
- Record the written neighbour order per atom and recompute tetrahedral
  parity on output, so @/@@ follows the molecule and not the spelling.
- Carry cis/trans marks on bonds and normalise them into a canonical
  frame anchored on the lowest-ranked substituent.
- Emit ring-opening digits at the atom itself (c1ccccc1), and bracket an
  atom whenever the bare form would imply the wrong hydrogen count.

Ring perception
- Keep the relevant cycles -- those not expressible as a GF(2) sum of
  strictly shorter cycles -- instead of a deduplicated per-bond scan.
  This matches RDKit's symmetrised SSSR: bicyclo[2.2.2]octane has three
  rings, adamantane four.

Descriptor definitions, matched to RDKit's own SMARTS
- H-bond acceptors: aromatic nitrogen only counts with two connections
  and no hydrogen; acid hydroxyls and amide nitrogens are excluded.
- H-bond donors: O/S need exactly one hydrogen and no charge.
- Rotatable bonds: full Strict pattern, including the trivial-rotor and
  conjugated-linkage exclusions.
- Crippen: test H3 before H2 so an aromatic N-H scores +0.2142.
- Murcko: retain exocyclic double-bonded atoms, replace severed bonds
  with hydrogens, and drop stereo only when the neighbour set changed.
- Formula gains its charge suffix; Li/Be/He gain monoisotopic masses.

SMARTS engine
- R<n> is ring-membership count, not "is in a ring".
- Support dot-separated multi-component patterns, which brings QED's
  last two structural alerts into play.

FFI
- ds_canonical_smiles returned -1 for a small buffer, ds_mol_formula and
  ds_add_hydrogens silently truncated. All three follow the sizing
  protocol now, and the fixed-size C++ buffers retry on the heap.

Morgan
- Share the common ring perception, break dedup ties by canonical rank,
  and avalanche the invariant before folding. Fingerprints are now
  spelling-invariant and pairwise Tanimoto tracks RDKit at rho > 0.95.

Tests
- scripts/gen_rdkit_fixtures.py regenerates the fixtures; 214 molecules
  and 1188 equivalent spellings under crates/smiles/tests/.
- 47 integration tests over descriptor parity, the canonical-form
  contract, Kekule/aromatic equivalence, scaffold rules, fingerprints
  and buffer contracts. Canonical and scaffold output is compared
  structurally rather than byte-wise, since two canonicalisers may
  legitimately choose different spellings of the same molecule.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@nkwork9999
feat: add structural molecular descriptors
f428ef1
@nkwork9999
fix: align structural descriptors with pinned RDKit reference
1d47958
@nkwork9999
fix: align extension CI with current DuckDB community toolchain
41538ee
@nkwork9999
fix: select Rust cross targets and satisfy extension formatting
3b5940d
@nkwork9999 nkwork9999 changed the title Reach RDKit parity across every scalar descriptor Improve RDKit descriptor parity and extension CI Sep 5, 2026
@nkwork9999 nkwork9999 mentioned this pull request Sep 5, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@nkwork9999