Port the two v3 C++ fixes: the singular-matrix solve and the detour-matrix hang - #14
Open
guillaume-osmo wants to merge 1 commit into
Open
guillaume-osmo wants to merge 1 commit into
guillaume-osmo wants to merge 1 commit into
Conversation
…atrix hang
Both fixes exist in the RDKit-integration line and neither was here. They are
ported as algorithms only -- this repo is the standalone Boost.Python module and
keeps its own single-file layout and wrapper conventions; nothing structural was
carried across from the split files.
**1. solveLinearSystem: dposv -> dgesv -> dgelss.** Two bugs, one function.
LAPACK modifies B in place even when it fails, so v1 retried dgesv on a corrupted
right-hand side. And a singular system had no solver at all: ethylene (`C=C`, a
singular 2x2) failed both tiers, the matrix descriptors fell through to a
5-element {0,0,0,0,0} stub instead of nrhs*5 = 20, and the molecule returned a
SHORT descriptor vector -- silently, and only for the molecules least likely to be
in anyone's test set. The RHS is now saved and restored before each fallback, and
dgelss provides a minimum-norm least-squares solution so the stub never fires.
**2. Detour matrix: biconnected decomposition.** The 14 *_Dt descriptors need the
longest simple path between every atom pair -- NP-hard -- and the naive
whole-graph backtracking DFS blows up exponentially on highly-cyclic molecules.
That is the hang the README documented as a known issue and shipped with.
Ported from 99e253646 (rdkit-brian PR #15), which follows Mordred's CalcDetour:
decompose into biconnected components, solve the longest path exactly within each
ring system (small), then merge along the block-cut tree through the shared
articulation atoms -- polynomial in the number of blocks. A per-block circuit-rank
> 12 guard returns NaN for a genuinely intractable fused cage, which does not
occur in real drug or natural-product chemistry. Upstream verified it
byte-identical to the previous algorithm on normal molecules (~1e-13); 12-22 ring
natural products go from indefinite hang to under half a second.
Not ported: the bounded thread pool and calcOsmordredWithTimeout. They wrap an
aggregate function, and this package has none -- molecule-level aggregation is
Python here. Bringing them over would mean designing a new C++ aggregate surface
plus bindings, which is its own change.
**Verification, and its limit.** `skbuild/test/test_v3_fixes.py` covers both:
full-width output for eight degenerate molecules across the five matrix blocks,
and termination plus unchanged values for the detour matrix. Widths are compared
against a reference molecule rather than hardcoded, so it holds across builds and
`version` values.
I could not compile it. This package needs conda + rdkit 2023.9.3 + boost 1.82,
which is not available here, so the C++ is checked by inspection only: the added
block is brace- and paren-balanced, the ported functions are byte-equivalent to
the upstream originals modulo the RDKit:: qualification this file uses, and every
API name the test calls is exported by __init__.py. The pre-existing paren
imbalance in this file (5893/5894) is on origin/main and is not from this change.
**Needs a real build before merge.**
The premise was checked against the installed RDKit-integrated build, which has
the dgelss fix: `CalcOsmordred` returns full width 3585 on C=C, C, CCCO and
benzene alike.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Stacked on #13 (base is
readme-structure, so this diff shows only the v3 work).Both fixes exist in the RDKit-integration line and neither was here. Ported as
algorithms only — this repo is the standalone Boost.Python module and keeps its
own single-file layout and wrapper conventions. Nothing structural was carried
across from Brian's split files; those are for upstream RDKit and should stay
separate.
1.
solveLinearSystem:dposv→dgesv→dgelssTwo bugs, one function.
Bin place even when it fails, so v1 retrieddgesvon acorrupted right-hand side.
C=C, a singular 2×2) failed bothtiers, the matrix descriptors fell through to a 5-element
{0,0,0,0,0}stubinstead of
nrhs*5 = 20, and the molecule returned a short descriptorvector — silently, and only for the molecules least likely to be in anyone's
test set.
The RHS is now saved and restored before each fallback, and
dgelssgives aminimum-norm least-squares solution so the stub never fires.
2. Detour matrix: biconnected decomposition
This is the hang the README documented as a known issue and shipped with. The
14
*_Dtdescriptors need the longest simple path between every atom pair — theNP-hard longest-path problem — and the naive whole-graph backtracking DFS blows up
exponentially on highly-cyclic molecules.
Ported from
99e253646(rdkit-brian PR #15), which follows Mordred'sCalcDetour:decompose into biconnected components, solve the longest path exactly within each
ring system (small), then merge along the block-cut tree through the shared
articulation atoms — polynomial in the number of blocks. A per-block circuit-rank
Upstream verified byte-identical to the previous algorithm on normal molecules
(~1e-13); 12–22 ring natural products go from indefinite hang to under half a
second.
Not ported, deliberately
The bounded thread pool and
calcOsmordredWithTimeoutwrap an aggregatefunction, and this package has none — molecule-level aggregation is Python here.
Bringing them over means designing a new C++ aggregate surface plus bindings.
That is its own change, not a port.
skbuild/test/test_v3_fixes.pycovers both fixes: full-width output for eightdegenerate molecules across the five matrix blocks, and termination plus unchanged
values for the detour matrix. Widths are compared against a reference molecule
rather than hardcoded, so the test holds across builds and
versionvalues.I could not compile this. The package needs conda + rdkit 2023.9.3 + boost
1.82, which is not available in this environment, so the C++ is checked by
inspection only:
RDKit::qualification this file uses ✓__init__.py✓origin/main, not from this change ✓This needs a real build before merge. Please run
build.shandtest_v3_fixes.py— I would not merge C++ on inspection alone.The premise was checked against the installed RDKit-integrated build, which
already has the
dgelssfix:CalcOsmordredreturns full width 3585 onC=C,C,CCCOand benzene alike — i.e. the singular-matrix truncation is genuinelygone once the tier is present.
README
A Versions, and the two codebases section, which keeps the standalone package
and the RDKit-integration line explicitly apart — layout, aggregate API, and which
is the curated line — plus a v1/v2/v3 table and the mechanism behind each fix. The
pathological molecule moves out of Known issues, since it is no longer one.
🤖 Generated with Claude Code