madspace: fix overweight tail after MadNIS training (truncation budget seeded with full target) - #191
Merged
Merged
Conversation
ae5a7f2 seeds every channel that enters generate() unsurveyed with the full run target, so the truncation budget of its first batch is max_overweight_truncation * target_count instead of that times the channel's share. _max_weight only rises and a channel that has met its (real, smaller) target is never revisited, so the too-low cap chosen in that first batch was permanent. After MadNIS training the generator is rebuilt unsurveyed, so every MadNIS run hit this: p p > t t~ j j had 0.26-0.30% of the cross section in overweights instead of <= 0.1%. set_target_count() now re-applies the truncation budget when the target decreases. That can only raise _max_weight, and unweight_all() re-unweights the whole weight file against the final cap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
theoheimel
self-requested a review
September 21, 2026 15:03
Contributor
|
We should also add a test to check that the truncation stays below the limit to prevent this from regressing again |
Regression test for the MadNIS truncation-budget bug fixed in the previous
commit. The systematic cross-section checks already generate 10k unweighted
events per process, so the check reads their events.lhe.gz instead of adding
a new (slow) generation to the CI.
The quantity is the fraction of |sigma| carried by events with
|w| > <|w|>: an unweighted event carries its channel's cap unless its raw
weight was above it, so this is what max_overweight_truncation bounds. The
limit is MG7_XSEC_OVERWEIGHT_FACTOR (default 2.5) times the run card's
max_overweight_truncation, i.e. 0.25% by default.
At 10k events (4 processes, one run each):
before fix with fix
p p > t t~ j j 0.48% 0.07%, 0.09%
p p > e+ e- j j 0.60% 0.12%
g g > t t~ g g 0.11% 0.10%
p p > t t~ j 0.04% 0.05% (MadNIS off: unaffected)
The tail is also recorded in the result JSON and shown as a column of the
job summary.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
theoheimel
approved these changes
Sep 23, 2026
theoheimel
left a comment
Contributor
There was a problem hiding this comment.
Looks good, let's merge this
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.
Problem
With MadNIS on, unweighted samples carry about 3× the intended overweight tail. With the
weighthistogram from #190,p p > t t~ j j(13.6 TeV, 100k events, default card) has 0.26–0.30% of the cross section above w/⟨w⟩ = 1, reaching 3.2.max_overweight_truncation = 0.001is meant to cap that at ~0.1%.It looked multiplicity-dependent, but it isn't.
p p > t t~ j jwith[madnis] enable = falsehas the smallest tail of the four runs:t t~ j, MadNIS off (default)t t~ j, MadNIS ont t~ j j, MadNIS on (default)t t~ j j, MadNIS offThe two processes differ only because
set_madnis_auto_settingsalways enables MadNIS forn_out >= 4, but forn_out == 3only when rsd > 10, events > 1M, or a gridpack is written. So a 2→3 process in those cases is affected too.Cause: introduced by ae5a7f2
ae5a7f2 ("max weight determination bugfix", 3 Sep 2026, merged to
mainon 7 Sep via #51) added this at the start ofEventGenerator::generate():The truncation budget in
update_max_weightismax_overweight_truncation × min(count_target, freeze_max_weight_after). Seeding the full run target makes each seeded channel's first-batch budget 100 (for 100k events) instead of 0.001 × its share. Instrumentingupdate_max_weightshows what happens to channel4.1oft t~ j j(final target 291, real budget 0.29): one 1000-event batch at budget 100 moved its cap from ~160 to 1.53. After that:_max_weightonly ever rises;generate()skipped it for the rest of the run andupdate_max_weightwas never called on it again.That cap was therefore permanent. Four such small channels (0.74% of the events) produced 61% of the whole overweight tail.
With the seed removed (the logic before ae5a7f2), the same MadNIS run gives 0.093%, so this commit accounts for the entire regression. I could not reproduce the case ae5a7f2 was fixing: without the seed the efficiency was 0.124 against 0.117 with it. This PR therefore keeps the seed and fixes what it leaves behind.
Why MadNIS off was not affected
The seed only fires for channels with
count_opt == 0whengenerate()starts.generate()runs on the sameEventGeneratorthat ransurvey(). Every channel already hascount_opt > 0, andsurvey()ended withupdate_integral_fractions(), which set real per-channel targets. The seed never fires. Budget excursions during the survey do not carry over either: each VEGAS batch callsclear_events(), which resets_max_weightand_large_weights.build_madnis()builds newChannelobjects (event_generator = None), andtrain_madnis()then callsbuild_event_generator()without a survey. So every channel is a freshChannelEventGeneratorwithcount_opt == 0, and the seed fires for all of them. Flows also have no VEGAS/discrete optimizer, soneeds_optimization()is false andclear_events()never runs to reset the cap.This rebuild-without-survey has been there since be0fe9f (2025) and was harmless before ae5a7f2.
Fix
ChannelEventGenerator::set_target_count()moves out of the header. When the target decreases, it re-applies the truncation budget (the existing loop, factored intoapply_truncation_budget()). This can only raise_max_weight. Raising it resets_unweighted_count, sounweight_all()re-unweights the whole weight file against the final cap, and the per-channel invariant (excess ≤max_overweight_truncation × count_target) holds whatever the seed was. The freeze is respected (no update oncecount_unweighted > freeze_max_weight_after).I also tried seeding with
target_count / n_channelsinstead. It gives 0.104%, but small channels still end up above their budget (up to 14×), so I did not include it. Combining the two gives 0.094%, which adds nothing over this change alone.Validation
p p > t t~ j j, defaults (MadNIS on): tail 0.26–0.30% → 0.096%, reach 3.2 → 2.x. Unweighting efficiency 0.117 → 0.120, σ unchanged (256.06 ± 0.19 pb). With the instrumented build, no channel exceeds its budget (worst 0.43×, was 70×).p p > t t~ j, defaults (MadNIS off): 0.087% → 0.093%, efficiency 0.082 → 0.084, σ 411.98 pb: unchanged within run-to-run noise.freeze_max_weight_afterand the defaultmax_overweight_truncationare not involved (no channel reached the freeze in any run). Neither needs to scale with multiplicity.Large batches (GPU): no return of the bug ae5a7f2 fixed
Before ae5a7f2, a channel entering
generate()unsurveyed keptcount_target = 1. Its first batch therefore ran with budget ≈ 0, and its cap became the maximum weight of that whole batch. Since the cap only rises, that choice was permanent, and with a 64k GPU batch it is very conservative. This PR keeps the seed and only ever raises caps, so I checked that it does not drift back to that behaviour. The first batch after MadNIS is one device batch, socpu_batch_size = 64000reproduces the GPU case on CPU. Three modes, all at 64k:p p > t t~ j j,cpu_batch_size = 64000main(seed with the full target)t t~ j, MadNIS off: 10–30%). So the caps come from each channel's real budget, and nothing is left of the conservative first-batch maximum.mainat 64k (0.053 against 0.064) only becausemainspends 4× its truncation budget. At 64kmainis also worse than at 1k (0.43% against 0.30%), because the budget-100 first batch spans 64× more events.t t~ jpath shows the same drop (0.084 → 0.067), identically in all three modes. That path never applies the seed, so this PR does not change it.🤖 Generated with Claude Code