Skip to contents

The review 04: the software-development expert (an independent review, 2026-09-28)

Source: vignettes/articles/review-04-software-engineer.qmd

The reviewer persona: staff engineer / open-source scientific-software maintainer. Method: line-by-line cross-check of the numbers inside the documents, the acceptance criteria and the internal consistency. The overall verdict: the direction is right and the research quality is exceptionally high, but this is a “paper wishlist”, not yet a delivery plan. Conditional pass — do the scope surgery and the three contract patches first, then write the first line of code.

P0

1.P0 The ROADMAP’s total workload is systematically underestimated by ~3×, and there is no “minimal adoptable v1” cut line anywhere

  • M0 in half a week to a week is to deliver 7 S7 classes + the registry + the contract tests + dual-end channel generation + the 2bit fixtures + three-platform CI — 1–2 weeks at the very least. M1: the 8 numerical kernels + the five channel-arithmetic suites + the ID83 and ID89 dual standards + the twobit reader + the gold-standard fixtures — “2–3 weeks” is a fantasy (really 6–8 weeks). The DM mixture-model replication of M3 and the conformal of M6 are mini research projects, not to-do items. M8’s “1 week” to finish the vendor + r-universe + the nf-core PR + the CRAN submission: CRAN’s review of a vendored Rust package is itself weeks of wall-clock iteration. The 10 vignettes have no milestone claimant.
  • The fix: a dual-track regime — v0.1 (~6 weeks): the SBS96/192/384/1536+DBS78 catalogs, the KL-NMF+NNLS two kernels, ms_read_*/ms_tally/ms_extract single method + the full CI + 3 golden fixtures, published on r-universe; v0.2 (~12 weeks): ID83, ms_select_k (stability + silhouette; SUITOR optional), ms_fit (nnls/likelihood/lrt), bootstrap CIs, refdb v1 (human SBS/DBS/ID), the 6 viz figures, the extending vignette; v1.0 (~5–6 months): the MSU-Fit coverage calibration, ms_hrd_report, pkgdown, the NEWS discipline. Deferred to v1.x: ID89, Infer/Tempo/Tensor, MsLda/MsSupervised, CN/SV fitting, therapy_forensics, the RNA workflow. Paper ③’s “pick 3–4 and do them deep” becomes a hard Non-goal.

2.P0 “Bit-for-bit replication / bit-level alignment” conflates two classes of acceptance, and contradicts its own licensing policy

  • (a) The channel semantics (labels/indices/enumerations/tie-break rules) — discrete semantics, genuinely bit-for-bit golden-verifiable, and it must be; (b) the floating-point kernels (KL-MU/NNLS/LRT) — bit-level equality depends on the operation order and the BLAS behavior, inherently fragile. The sharper internal contradiction: the bit-equivalence of rust-NMF is a ported implementation (and thus it inherits GPL≥2); D1 promises a “clean re-implementation from the paper formulas” — to get bit-equivalence you must replicate the operation order, and how much replication counts as a derivative work is a grey zone. The acceptance criteria and the licensing policy collide head-on, and nobody in the archive adjudicates it.
  • The fix: §7 explicit bifurcation: “the semantic bit-for-bit” (bit-compatible: the channel arithmetic / the protocol steps / the tie rules / the default parameters) = a hard acceptance, the integer and label golden vectors; “the numerical protocol equivalence” (protocol-equivalent: the floating-point kernels) = fixed seed + tolerance (cosine > 1−1e-6, exposure L1 < tol). The R NMF brunet cross-check = a one-off fixed-platform verification experiment, not into the CI gate. Delete every unqualified “bit-for-bit” in the ROADMAP and tag each one with its category.

3.P0 The determinism claims lack thread-count invariance — the headline claim will be silently pierced by the parallelization

  • The seed forking solves the random-number streams, not the floating-point reduction order; the consensus clustering / the bootstrap quantiles / the multi-initialization best-picking all involve floating-point accumulate-and-compare. The CI checks run ≤2 threads vs the user’s 16 — the determinism claim was never designed for.
  • The fix: (a) the engine contract tests add a “seed fixed, RAYON_NUM_THREADS ∈ {1,2,N} outputs identical” assertion; (b) parallelism slices only along dimensions that do not reduce into one another, and the cross-replicate statistics are computed single-threaded in a fixed order after collection; (c) the provenance records the thread count but the promise is that the result is independent of it.

P1

4.P1 The refdb dual source (bundle + cache) has no precedence, forward-compatibility or integrity model

  • The precedence is undefined (is it the bundle or the cache by default?); the schema compatibility in the degradation scenario; the update source pinned to a raw HEAD that can be rebuilt without a version number (SPMG has demonstrated it), and comparing the SHA256 of the same channel is self-certification, not validation; the volume audit is a release-critical path, not a background task.
  • The fix: (a) an explicit precedence: “an analysis is forever pinned to the version recorded in the object’s provenance; by default the bundled one prevails, the cache only on an explicit request”; (b) the metadata gains a schema_version, and > the package-supported value means stop; (c) updates pinned to a commit SHA + a build-time burned-in manifest (the r-polars lib-sums.tsv pattern), fail-closed; (d) all update-path tests fixture-ized (CRAN forbids networking during checks); (e) v1 bundles only the human SBS/DBS/ID, the rest goes through the cache, aligning with the 5MB guidance line.

5.P1 The GRCh38 “derivation strategy” is scientifically undecided — a relabel or a derivation, the archive has not thought it through

  • The labels of these channels are themselves build-irrelevant — if the labels are irrelevant, a “conversion” of what? The matrix is the same numbers, so that is a relabel, not derived; and if one believes the build does matter, a label-level conversion simply cannot manufacture a legitimate matrix.
  • The fix: an (a)+(b) hybrid: declare “the ID/CN/SV/RNA-SBS channel semantics and the signature matrices are treated as build-irrelevant (build_independent: true), and a missing build reuses the same matrix” + an honest notice at query time: “this build has no official XX reference set”.

6.P1 The API contract is unfinalized and the documents already fight each other; the error-message and lifecycle policies are wholly absent

  • §1 says method = "nnls/qp/..." strings, D5/§3.4 say the engines are S7 objects — the v1’s number-one API decision has two chapters each speaking their own talk; MsEngine.mode = class_character is itself a magic string; extract_signatures is a dangerous name inside a signature-extraction package (it collides with hardhat); the error-message policy (the rlang class, the i/j/c three-part form), the lifecycle policy and the FFI export freezing procedure are all missing.
  • The fix: (a) nail it down: ms_extract(catalog, method = ms_nmf(...)) the object as a first-class citizen + the method = "nmf" string sugar (match_ms_engine() lists the available engines on error); (b) the custom generics uniformly ms_-prefixed (the hardhat naming stays only a documentation-concept correspondence); (c) a new §3.5 “Error & lifecycle”: the error-class convention (msuiter_error_catalog), a lint forbidding bare stop(), lifecycle::deprecate_warn, the NEWS as the changelog, vignettes/articles/ffi-surface.qmd as a per-release review artifact.

7.P1 The CI matrix has hard gaps: the declared R ≥ 4.3 floor is not in the test matrix; the FFI memory safety has zero coverage

  • oldrel does not cover 4.3/4.4 either — a floor = an untested declaration; extendr FFI + externalptr + memmap2 is exactly the combination that most needs valgrind/ASAN-UBSAN; the GHA shared-runner noise will make the benchmarking-threshold alarms false-fire every week (a “≥50×” is unverifiable on GHA); there is a cargo deny but no cargo audit/dependabot; the coverage has no target value; the golden fixtures have no refresh-workflow carrier.
  • The fix: the matrix becomes 3 OS × {release, oldrel, R-4.3 (Linux at least), devel (allowed to fail)}; add the two scheduled jobs valgrind + sanitizers; the benchmarking gate downgrades to a sanity bound (<5s), the speedup-ratio acceptance moves into the bench/ pinned-hardware procedure; the dependabot triad; codecov dual-language (R ≥85%, Rust reports with no gate); data-raw/refresh_fixtures.R + a PR-template forced diff-review check item; the rhub/win-builder/–as-cran pre-submission discipline.

8.P1 The governance and supply-chain model of the third-party engines is undefined: a “unified quality gate” is unenforceable on external engines

  • Registration is code execution — a third-party .onLoad() injects an arbitrary fit_fn, and ms_benchmark() calls it with the user’s data; you have no mechanism to force a third party through the contract tests (the tests run in your CI, and their updates you will not know of); the registry has no contract_version/engine_version/certified fields; CONTRIBUTING/CoC/templates are all missing.
  • The fix: (a) the registry schema gains engine_version/contract_version/certified (certified/self-reported/unknown); (b) ms_benchmark() emits a warning for non-certified engines + the documentation states the trust boundary; (c) a monthly reverse-dependency compatibility matrix job; (d) M0 adds the CONTRIBUTING (with the third-party-engine onboarding checklist), the CoC and the issue/PR templates.

P2

9.P2 ARCHITECTURE and ROADMAP have already drifted (ms_therapy_forensics evaporated); the ADR form cannot hold up the evolution

  • The two v2 documents, written just now, are already not synchronized. The fix: fix the contradiction or explicitly demote to a Non-goal; extract D1–D10 into vignettes/articles/NNNN-*.md (with status/superseded fields); the research documents’ headers gain an “archival snapshot” declaration; add a docs-sync CI (extracting the ms_* from the ROADMAP and cross-checking against the ARCHITECTURE/R layout, any drift turns it red).

10.P2 “One package + splittable later”: the Rust-side boundary is real, the R-side boundary is a wish

  • The Rust three-crate split is compiler-enforced; the R-side R/utils.R will become a sewer in six months. The fix: the CI adds a cargo-workspace dependency-direction assertion; an R-side architecture test (R/<section>/ may call only its own section + classes/ + the FFI wrappers + the whitelist); each section gets a three-line README declaring the dependency direction; concretize the “package-split line” as the sole candidate cut surface (the engine crate published on crates.io; the R package split only as a contingency plan for when the CRAN size is rejected).

11.P2 The S7 object model’s hidden costs: the heavy embedding, the handle-reconnect UX, the 0.3.x migration

  • MsSignature embeds a whole MsCatalog (a thousand-sample cohort = N catalog copies of memory duplication) + the waldo comparison noise; the handle-reconnect UX is undesigned (what is re-run? how long? when triggered?); the S7 0.2→0.3 migration goes unmonitored.
  • The fix: the catalog/reference properties become snapshot summaries (the dimensions + the channels-registry hash + the refdb version), the full objects passed explicitly; the M0 acceptance adds “after saveRDS/load every generic’s behavior has a test”; a nightly job on the S7 main branch.

12.P2 The engineering hygiene and the release engineering are missing: NEWS/semver, README/pkgdown, sitrep, the MSU-* ↔︎ function mapping, the performance anchoring

  • The fix: M0 adds a README skeleton + NEWS.md + _pkgdown.yml; the ms_sitrep() diagnostic function (the R version / the Rust toolchain / the rayon threads / the refdb state / the 2bit cache); the MSU-* ↔︎ exported-function mapping table into the pkgdown; M7 adds the CITATION + the performance-anchoring documentation (the pinned hardware / the dataset version / the competitor versions); split M8 into “M8a the materials week” + “M8b the CRAN iteration (wall-clock 4–8 weeks rolling)”.

The overall verdict and the highlights

  • Conditional pass. None of the three P0s costs money, only decision discipline: the collision of the bit-for-bit acceptance with the clean-room, the conflict of the parallelization with the determinism, the 15–21-week fairy tale without a v1 cut line.
  • The three things done well: the fact-check audit culture (institutionalized it becomes a moat); the D9 comparison/component separation (answering both the “it is just a wrapper” challenge and the extensibility — the most mature product decision of the whole archive); the measured numbers turned into engineering iron laws (staff-grade judgment).