Build against RDKit 2026.03, harden the C++ core, modernize the Python API - #6
Merged
Conversation
Make the C++ extension build reliably against the latest RDKit and drop the hardcoded, machine-specific paths that made it fragile. Build: - setup.py: auto-detect RDKit from RDKIT_PREFIX / CONDA_PREFIX / sys.prefix (works whether or not the env is activated); handle both the conda-forge include/rdkit/ and the plain include/ header layouts; bake an -rpath to the env lib dir so RDKit dylibs load without DYLD_LIBRARY_PATH / PYTHONPATH hacks; remove the hardcoded /Users/... boost path and the 6-heuristic search. Project metadata now lives in pyproject.toml; setup.py only declares the extension. - cxx_std 17 -> 20: RDKit 2026.03 headers require C++20 (constexpr virtual, etc). - environment.yml: one-command conda-forge env that pulls the dev packages the build actually needs and that previously tripped people up -- librdkit-dev (C++ headers) and libboost-devel (Boost headers), plus cxx-compiler -- not just `rdkit` (which is runtime-only). - pyproject.toml: real repo URLs (were `yourusername`) + package discovery. Docs: - BUILD.md: quick start, detection order, the conda-forge dev-package split, custom-RDKit (RDKIT_PREFIX) path, and a troubleshooting table. - README: one-command install, C++20 + RDKit 2026.03 badges, `pip install -e .`. Cleanup: - Archive 17 stale PR-body / phase-summary / log files from the repo root into docs/dev-notes/ to declutter the top level. Verified: builds against RDKit 2026.03.3 in a clean conda-forge env, then imports and featurizes with no runtime path hacks.
transform() under method='dummy_masking' demanded train_indices_per_task, and those indices were treated as rows of the fitted *training* set -- but they actually index into the `smiles` batch being transformed. On a held-out batch this re-derived the "train keys" from the wrong rows, silently collapsing the features (and indexing out of bounds for any smaller or brand-new batch). That was the inference-collapse bug. - Out-of-sample inference is now transform(smiles) with NO train_indices: it uses the frozen fitted prevalence, and keys unseen in training are absent from the maps and contribute 0 -- i.e. dummy-masking at inference, with no batch-relative indices required. - When train_indices_per_task IS supplied (in-sample CV masking), out-of-range indices now raise a clear error instead of collapsing silently. - examples/example_ml_xgboost.py: use transform(test/new_smiles) for inference instead of passing train_indices that belong to the fitted set.
Real bugs surfaced while building against RDKit 2026.03 and reviewing the core. - k_threshold is now a real, functional parameter. It was stored in Python but never passed to C++ (hardcoded to 2 since 815f951), so the rare-key filter was stuck at 2. Added it to the MultiTaskPrevalenceGenerator C++ constructor + pybind init and wired the Python passthrough; it now changes which keys survive (and therefore the features). - chi2/chisq aliasing: the sequential build_1d_ftp_stats matched only "chisq", so the default stat_1d="chi2" fell through to a legacy Fisher/Woolf z-test while the threaded path computed Pearson chi-square -- same test_kind, two different statistics depending on the path. Sequential now aliases "chi2" -> chi-square, matching threaded. - pickle / save-load: __getstate__/__setstate__ were bound as separate methods, so pickle.dumps/loads (and save_features/load_features) returned an UNFITTED object. Switched to the proper py::pickle factory; round-trip now restores a fitted, transform-identical one. - indexed-vs-legacy miner determinism: the indexed pair miner used a non-stable std::partial_sort, so among equal-Tanimoto FAIL candidates it chose a libc++-dependent partner. Added an explicit tie-break on the original FAIL index. - quiet by default: fit()/transform() printed banners to stdout on every call regardless of verbose. Gated all diagnostic cout on verbose_, and moved the n_measured==0 check ahead of the percentage prints (fixing a latent divide-by-zero). - loo_smoothing_tau: it is not implemented in the C++ core; the Python layer now warns when it is set to a non-default value instead of silently ignoring it. Tests: conftest updated to the real C++ constructor API (use_key_loo, k_threshold); the pickle test uses real pickle.dumps/loads; the miner equivalence test uses chemically distinct molecules (greedy matching on identical molecules is inherently order-dependent); tau and the currently no-op Key-LOO rescale are honestly marked skip/xfail with reasons; new test_regressions.py pins each fix. Full suite green (skip+xfail documented).
…docs
Modernize the package so feature generation (inference) and label prediction (predict) are
distinct, intention-revealing modules:
inference molftp.features SMILES --fit/transform--> feature vectors
predict molftp.predict SMILES --features--estimator--> labels / probabilities
- molftp/predict.py: new MolFTPClassifier -- a scikit-learn-style estimator
(fit / predict / predict_proba / transform) that composes a molFTP feature generator with
any sklearn estimator (default LogisticRegression). transform() is inference-only (features,
no labels). Input validation with clear error messages.
- molftp/features.py: re-exports the prevalence generators as the inference layer so the two
layers have distinct import paths.
- molftp/__init__.py: exports both layers with a docstring describing the split.
- docs/api.md: full API reference -- the two layers, key_loo vs dummy_masking, parameter
semantics (especially k_threshold), and honest limitations.
- README: end-to-end prediction example, docs links, corrected Key-LOO description.
- tests/test_predict.py: predict-layer behaviour (shapes, custom estimator, error paths).
… inert
- tests/test_model_mw.py: end-to-end learning test. Builds 200 diverse molecules, sets a
binary target by thresholding RDKit molecular weight at the median, and asserts
MolFTPClassifier reaches AUC > 0.70 (observed ~0.87) on a held-out split -- i.e. the
inference->predict pipeline genuinely learns structural signal, not just shapes.
- Resolve the two previously-flagged points; root cause is shared. molFTP's 3-view features
are SIGN-BASED net counts (build_3view_vectors_batch counts atoms with prevalence >= 0 vs
<= 0). Only the sign matters, never the magnitude, so:
* the Key-LOO (k_j-1)/k_j rescale and loo_smoothing_tau's (k_j-1+tau)/(k_j+tau) are
positive scalars that preserve sign and thus cannot change the features -- inert by
design, not a bug;
* the effective rare-key / leakage control is k_threshold, which removes keys (and so
does change the counts).
The former xfail becomes a passing characterization test
(test_positive_rescale_is_inert_for_sign_count_features) pinning the invariant; the tau
skip reason, the Python RuntimeWarning, and docs/api.md now state the real reason. Making
the LOO magnitude correction matter would require magnitude-aware aggregation -- a
deliberate change to all downstream results, left to the maintainer.
…LOO decision Records the paper-vs-code analysis behind the inert Key-LOO rescale / loo_smoothing_tau: - The paper (arXiv:2510.06029, eq. 5 + "MolFTP vector") defines the margin feature as max(positive) - min(negative) -- magnitude-based. The default "max" code path computes a sign-count (p - n) for the margin/relative-margin instead; the proportion features (net_0..net_R) do match the paper. - The paper's own Figure 6 (dummy-masking ablation: mu=0.000, sigma=0.000 on the proportion vectors) empirically confirms the magnitude rescale is inert on sign-count features; key-LOO (singleton removal == k_threshold) is what actually moves the vectors. - Decision (a leave / b delete / c magnitude-aware margin) cannot be made from the paper alone: the headline numbers (Table 2, XGBoost key-LOO AUROC 0.9053 / AUPRC 0.9490) came from the current sign-count code, so option (c) must be re-benchmarked on BBBP before adoption. docs/research-notes.md captures the proposed experiment; docs/api.md links to it.
…w margin The margin features V[0]/V[1] were sign-counts, but the paper (arXiv:2510.06029 eq.5) defines them as the magnitude max(+)-min(-). Rather than change the de-facto method, make it configurable: - margin_mode='signcount' (default, backward-compatible; reproduces the published numbers) - margin_mode='magnitude' (paper eq.5: max positive - min negative atom-localized score) - margin_mode='both' (concatenate signcount + magnitude, +2 features per view) Wired through the C++ core (build_3view_vectors_batch via a margin_mode_ member propagated to each task generator; get_features_per_task; constructor; pybind; pickle with 21/22-tuple back-compat) and the Python wrapper (string->int mapping, save/load round-trip). Benchmark (5-fold, R=6, key-LOO; ratio/magnitude/concat x LR/RF/mlxTM on BBBP/MDR1/MOR): magnitude >= concat >= ratio for LR/RF -- magnitude is best or tied-best in 6/8 LR+RF cells, small but consistent (~+0.005-0.01 AUROC); mlxTM is mode-agnostic (thermometer binarization). The sign-count numbers reproduce the paper's Table 2. Full table + decision in docs/research-notes.md. Default stays 'signcount', so existing behaviour is unchanged. Coverage: tests/test_kloo_core.py::test_margin_mode_option. Suite: 41 passed, 1 skipped.
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.
Cleans up the build, moves to the latest RDKit, fixes several real correctness bugs in the C++
core, and reorganizes the Python API into clear inference/predict layers. Full suite: 40 passed,
1 skipped (against RDKit 2026.03.3, macOS/Apple-Silicon conda-forge env).
Build (RDKit 2026.03, C++20)
setup.pyauto-detects RDKit fromRDKIT_PREFIX/CONDA_PREFIX/sys.prefix; handles bothconda-forge
include/rdkit/and plain layouts; bakes an-rpathso dylibs load withoutDYLD_LIBRARY_PATH/PYTHONPATHhacks; drops the hardcoded/Users/...boost path.cxx_std17 → 20 (RDKit 2026.03 headers use C++20).environment.yml: one-command env that pulls the dev packages the build actually needs —librdkit-dev(C++ headers) +libboost-devel— not justrdkit(runtime-only). This was theFix/key loo v1.3.0 #1 cause of "headers not found".
BUILD.md+docs/api.md; archived 17 stale root files intodocs/dev-notes/.Correctness fixes (C++ core)
k_thresholdis now functional — it was stored in Python but never passed to C++ (hardcodedto 2). Added to the constructor + pybind init; it now changes the features.
chi2/chisqaliasing — the sequential 1D path matched only"chisq", so the defaultstat_1d="chi2"silently used a Fisher test while the threaded path used Pearson chi-square.Now consistent.
save_features—__getstate__/__setstate__were bound as separate methods, soround-trip returned an unfitted object. Switched to
py::pickle().std::partial_sortpicked libc++-dependentpartners among Tanimoto ties; added an explicit tie-break.
fit/transformcoutbanners onverbose_; fixed a latentdivide-by-zero.
Python modernization
molftp.features(inference: SMILES→features) and newmolftp.predict(
MolFTPClassifier, sklearn-stylefit/predict/predict_proba/transform).Tests
test_predict.py,test_regressions.py(pins every fix), andtest_model_mw.py— anend-to-end molecular-weight-threshold classification on 200 diverse molecules (AUC ≈ 0.87).
Open research question (see
docs/research-notes.md)loo_smoothing_tauand the Key-LOO(k_j−1)/k_jrescale are inert because the default"max"feature path computes sign-counts, while the paper (arXiv:2510.06029, eq. 5) defines the
margin as
max(+) − min(−)(magnitude). The paper's own Figure 6 confirms the rescale is inerton the proportion vectors, and
k_threshold(singleton removal = key-LOO) is the real leakagelever. Deciding whether to make the margin magnitude-aware needs re-benchmarking on BBBP, since the
paper's headline numbers came from the current sign-count code.