fix(chrombpnet): BPNet CHIP — joint strand softmax, exp(C)-n_tracks, bias shapes - #120
Merged
Merged
Conversation
…bias shapes Three separate defects in the BPNet/CHIP path, all of which had to be settled together because each changes what the 744 CHIP CDF rows must be built from. The convention question is answered from the vendored TRAINING code, not by analogy: chorus/oracles/chrombpnet_source/templates/BPNet/ is upstream kundajelab/bpnet-refactor (losses.py byte-identical; arch.py and custommodel.py differ only by import shims). 1. JOINT strand softmax, not per-strand. custommodel.py:93-104 (the orig_multi_loss=False branch) reshapes the (B,1000,2) profile to (B,2000) and applies ONE multinomial_nll, and losses.py normalizes over the last axis. So the trained profile parameterization IS a single softmax spanning both strands. The oracle softmaxed each strand separately and scaled EACH by the full count total, making the two emitted tracks sum to 2.00x what the model predicted (measured exactly 2.00x). The builder did a third thing again - summed the strand LOGITS before one softmax - whose 501bp window sum drifted 0.84-1.30x versus per-strand across five loci, i.e. sequence-dependently, so the two could never be reconciled by rescaling. Which branch trained these weights is provable, not assumed: orig_multi_loss=False gives a (2,1) logcounts kernel, True gives (3,2) and REFUSES to load the real h5. All 744 cached BPNet models have (2,1). 2. The count inverse is exp(C) - n_tracks, not expm1(C). bpnet-refactor's generator builds the count target PER TRACK as log1p - np.log(np.sum(profile_predictions, axis=1) + 1) over an array shaped (batch, positions, tracks), so the sum is over positions - and custommodel.py:57 pools a task's tracks with reduce_logsumexp. The trained target is therefore log(n_tracks + total), so the inverse is exp(C) - n_tracks. For the 42 single-track ATAC/DNASE models n_tracks == 1 and this IS expm1(C): verified BIT-IDENTICAL (max diff exactly 0.000e+00) against the pre-change formula, so PR #113 stands and those 42 CDF rows stay valid. For the 744 two-track CHIP models expm1 left exactly ONE read of inflation - measured 1.12-1.95x on real loci, worse at quiet sites, median 1.78x on background 501bp window sums. Kept expm1 for n_tracks == 1 since it is more accurate for small C. (Upstream's own bpnet/cli/predict.py uses a bare exp(C), i.e. it is off by +n_tracks in the other direction. The target construction is the authority.) 3. The count-bias input shape was hardcoded (N, 1) for a (None, 2) input. BPNet logsumexp-reduces that bias before the final Dense (main_logsumexp_counts_bias_0), so the wrong width - which Keras silently broadcasts rather than rejecting - made the term log(1)=0 instead of log(2)=0.6931 and shifted every predicted log-count down by a constant w*log(2) = 0.588506 with w = 0.849035 (the Dense weight; matches the measured shift to all printed digits). Counts came out 1.80x too low at a peak and up to 3.04x at a quiet site. Profile logits were unaffected. The background builder derived the shape correctly and was NOT affected, so the oracle silently disagreed with its own CDFs. Fixed at all four call sites via _zero_bias_inputs, which derives the shapes from model.inputs: _predict_direct, predict_sliding, the env-path predict_template.py, and the public one-shot helper in oracles/bpnet.py (whose docstring also prescribed the superseded per-strand exp(counts) recipe). Also fixed: predict_sliding used SUM-LOGITS for CHIP while _predict used per-strand, and reported the result under a ':+' track_id. It now takes the joint softmax and emits the PLUS strand, matching the id it reports and the pooled per-strand CDF row it is looked up against. Emitting a both-strand total there would put 1.53-2.93x the _predict value behind the same id. Verified end-to-end on real weights: builder and oracle now agree to 2.0e-07 on every strand/locus combination, the two strands conserve exp(C)-2 exactly, and DNASE:K562 is bit-identical to before. CONSEQUENCE: the 744 CHIP CDF rows must be rebuilt against this transform; the 42 ATAC/DNASE rows must NOT (they are unchanged, and merge_shards appends and de-dups by track_id, so the base must contain only those 42 at merge time). Not yet rebuilt - see the PR. tests/test_chrombpnet_counts.py grew to 23 tests, including a helper that constructs the count target the way training does (per-track log1p pooled with logsumexp) so the expectations cannot drift from the loss, an explicit bit-identity test for the single-track inverse, and a check that expm1 overstates a two-track model by exactly one read. Fast suite: 491 passed, 4 skipped, 0 errors. 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.
Item C. You asked me to think harder before spending the 4 GPU-hours — that was the right call. The joint-softmax convention you picked is confirmed, but I found two further defects, one of them larger, that would have been baked into the rebuild. All three had to be settled together because each changes what the 744 CHIP CDF rows must be built from.
The convention question is now answered from the vendored training code, not by analogy:
chorus/oracles/chrombpnet_source/templates/BPNet/is upstreamkundajelab/bpnet-refactor(losses.pybyte-identical;arch.py/custommodel.pydiffer only by import shims).1. Joint strand softmax — confirmed, and now proven
custommodel.py:93-104(theorig_multi_loss=Falsebranch) reshapes the(B,1000,2)profile to(B,2000)and applies onemultinomial_nll, which normalizes over the last axis. The trained parameterization is a single softmax spanning both strands.Which branch trained these weights is provable, not assumed:
orig_multi_loss=Falseyields a(2,1)logcounts kernel;Trueyields(3,2)and refuses to load the real h5. All 744/744 cached models have(2,1).So per-strand isn't merely disfavoured — with a single count head there is no per-strand total to scale by, making it inexpressible.
2. The count inverse is
exp(C) − n_tracks, notexpm1(C)← the one I nearly baked inbpnet-refactor's generator builds the count target per track as
log1p:np.log(np.sum(profile_predictions, axis=1) + 1)over an array shaped(batch, positions, tracks)— so the sum is over positions — andcustommodel.py:57pools a task's tracks withreduce_logsumexp. The trained target is thereforeexpm1(C)exp(C) − nFor the 42 single-track models this reduces to
expm1exactly — verified bit-identical, max diff0.000e+00. So #113 stands and those 42 CDF rows stay valid. For the 744 CHIP modelsexpm1left exactly one read of inflation: measured 1.12×–1.95× on real loci, worse at quiet sites (median 1.78× on background 501 bp window sums).Upstream's own
bpnet/cli/predict.pyuses a bareexp(C), off by+n_tracksthe other way. The target construction is the authority, not that CLI.3. The count-bias input shape was hardcoded
(N, 1)for a(None, 2)inputBPNet logsumexp-reduces that bias before the final Dense (
main_logsumexp_counts_bias_0). The wrong width — silently broadcast by Keras, not rejected — made the termlog(1)=0instead oflog(2)=0.6931, shifting every predicted log-count down by a constantw·log 2 = 0.588506(w = 0.849035, the Dense weight — matching the measured shift to all printed digits).Profile logits were unaffected. The builder derived the shape correctly and was not affected, so the oracle silently disagreed with its own CDFs. Fixed at all four call sites via
_zero_bias_inputs(shapes derived frommodel.inputs):_predict_direct,predict_sliding, the env-pathpredict_template.py, and the public one-shot helperoracles/bpnet.py— whose docstring also prescribed the superseded per-strandexp(counts)recipe.Also fixed
predict_slidingused SUM-LOGITS for CHIP while_predictused per-strand, reporting it under a':+'id. It now takes the joint softmax and emits the plus strand, matching the id it reports and the pooled per-strand CDF row it's looked up against. A both-strand total there would be 1.53–2.93× the_predictvalue behind the same id.Verification
exp(C) − 2exactlyDNASE:K562bit-identical to before (0.000e+00)tests/test_chrombpnet_counts.py, including a helper that builds the count target the way training does so the expectations can't drift from the loss, a bit-identity test for the single-track inverse, and a check thatexpm1overstates a two-track model by exactly one readRebuild — not done yet, deliberately
The 744 CHIP rows must be rebuilt; the 42 ATAC/DNASE rows must not (unchanged, and
merge_shardsappends and de-dups bytrack_id, so the base must contain only those 42 at merge time). Expected CHIP counts double, since both strands now feed the same row: effect 37,344 / summary 68,008 / perbin 2,176,256.Two caveats worth your judgement before I start it:
(B,2)zeros is the right shape, but bpnet-refactor trains with real control bigwigs unlessset_bias_as_zerowas used, and which applies to the ENCODE-published BPNet models can't be determined from the weights. The 0.5885 uplift correctly encodes "no control" — it isn't necessarily the regime these models were trained in. Fine for percentiles (self-consistent), worth knowing before quoting absolute CHIP counts as calibrated._MIN_MAGNITUDE['tf_binding'] = 0.1(variant_report.py:594) becomes more load-bearing as CHIP values shrink, and a CDF rebuild doesn't address it.Say go and I'll run the CHIP-only rebuild and open the HF dataset PR with the same discipline as PR #2.
🤖 Generated with Claude Code