Skip to content

Extract the shared sampler/transform: 8 copies of the arithmetic is how the ChromBPNet CHIP bugs shipped #125

Description

@lucapinello

Why

All three ChromBPNet CHIP defects fixed on 2026-07-31 (#113, #120) were two copies of the same arithmetic disagreeing:

  • exp vs expm1 on the count head (4 call sites, fixed independently);
  • per-strand softmax vs one joint softmax over the flattened both-strand vector;
  • a hardcoded (N,1) count-bias against a declared (None,2) input, which Keras silently broadcasts rather than rejecting — making an internal logsumexp return log(1)=0 instead of log(2) and shifting every predicted log-count down by a constant 0.5885.

None of these were caught by tests, because the oracle and the builder each had their own copy and the tests pinned each copy to itself.

The duplication, measured

concern copies shared?
ReservoirSampler 8 (one divergent: AlphaGenome hand-vectorises add_batch, capacity 20k vs 50k; one truncated) no
one-hot encoding 6 in builders + 4 in the library, no two identical — (L,4) float32 vs (L,4) bool vs (4,L) vs (N,4,L) no
get_sequence 8, with silently different N-fraction thresholds (0.5 vs 0.3) no
TSS derivation 8, each reaching into the private AnnotationManager._get_genes_df while the public get_tss_positions sits unused no
compute_effect 3 byte-identical + 1 hardcoded + 3 inlined Cherimoya only
window scoring 3 identical get_window_slice + 2 score_window_sum in words only
cCRE positions an 11,500-position dict pasted ; sample_ccre_positions' own default (21,000) is dead code partially

The precedent to generalise

chorus/oracles/cherimoya_source/scoring.py is imported by both the oracle and its builder, and says so deliberately — "so that class of drift [is] a type error rather than a silent numerical bias." Two patterns there are worth copying wholesale:

  • tests/test_cherimoya.py:609 asserts the builder's source text contains the import, so the sharing cannot be quietly undone;
  • the builder validates its fast GPU path against the shared reference on the first batch (assert_allclose(..., rtol=1e-4)), so sharing costs no throughput.

How to land it

Extract-then-migrate, so a regression blames exactly one oracle:

  1. One PR adds the shared module and touches no builder.
  2. Then one small PR per oracle, each proving byte-identical sample counts before/after.

The sampler must take the position set as an argument rather than hardcoding it — otherwise it blocks the reference-class work in #83 and the deferred activity-stratified design.

Two latent defects the extraction should absorb

  • ChromBPNet silently halves its own effect CDF. The DHS-anchored SNP block is wrapped in except FileNotFoundErrorlogger.warning(...) (build_backgrounds_chrombpnet.py:450-451, again at :519). A build missing the vocabulary ships 9,609 effect samples instead of 18,672 and nothing fails. Cherimoya hard-fails in the same situation; that is the correct behaviour.
  • Only EPInformer-seq validates the ref-allele substitution (if seq[offset] != ref_base: continue). The other seven substitute at a fixed INPUT_LENGTH // 2 - 1 and trust it. The arithmetic is correct today — but with 8 divergent copies of get_sequence, a windowing change would corrupt backgrounds silently rather than raise.

Also worth fixing while in here: _DHS_VOCAB_SHA256 (chorus/utils/annotations.py:731) is declared and never enforcedhashlib appears zero times in that file — while a comment at build_backgrounds_cherimoya.py:275-278 tells the reader the file "is checksummed". The cCRE Registry is fetched by bare urllib.request.urlretrieve with no verification, and the DHS vocabulary via hf_hub_download with no revision=, so it tracks a branch head.

Related: #83, #113, #120.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions