fix: keep loop_Nc_power out of the process-wide colour memo - #84
Open
oliviermattelaer wants to merge 2 commits into
Open
fix: keep loop_Nc_power out of the process-wide colour memo#84oliviermattelaer wants to merge 2 commits into
oliviermattelaer wants to merge 2 commits into
Conversation
LoopColorBasis does not call ColorBasis.__init__, so _canonical_dict stays the class-level dict and is shared by every loop matrix element in the process. The cached ColorFactor carried loop_Nc_power, which is set per diagram by closeColorLoop and depends on compute_loop_nc - not on the canonical color string the memo is keyed on. The first generation to fill an entry pinned its loop_Nc_power for the process lifetime, so a compute_loop_nc=False run (e.g. [sqrvirt=] + output standalone) stored None and any later loop-induced madevent output hit it and crashed in get_icolamp_lines with 'int - NoneType'. The color-algebra simplification is a pure function of the canonical string, so drop loop_Nc_power when storing and stamp it back from the incoming string on every path. full_simplify only ever copies/multiplies strings, so it propagates loop_Nc_power unchanged - the stamp is a no-op on the miss path and restores the correct value on a hit. The memo stays shared. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Unit test drives two LoopColorBasis objects through the shared memo with different loop_Nc_power; acceptance test runs the real two-generation reproducer ([sqrvirt=] standalone then [noborn=] madevent) in one process. Both fail on the parent commit. Registered in the existing CI slots. 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.
The bug
One loop generation poisons every later one in the same process:
Pre-existing; reproduced on
28fe3cef1(currentmain, after the 3.7.3 merge).Each generation on its own works fine.
Root cause
ColorBasis._canonical_dict(madgraph/core/color_amp.py:51) is a classattribute — a memo from a canonical colour string to its simplified
ColorFactor.ColorBasis.__init__shadows it with a fresh instance dict, butLoopColorBasis.__init__never calls its parent's__init__, so everyLoopColorBasisin the process reads and writes the class-level dict.The cached
ColorFactor's strings carryloop_Nc_power, whichcreate_copy(color_algebra.py:940) copies out on a hit.loop_Nc_poweris set per diagram byLoopColorBasis.closeColorLoopanddepends on
compute_loop_nc: an integer when it is true,Nonewhen it isfalse (
loop_color_amp.py:104, deliberately a sentinel). It is neither part ofthe cache key nor recomputed on a hit, so whichever generation fills an entry
first pins its
loop_Nc_powerfor the process lifetime.Instrumented on
main:LoopColorBasisthat fills the memoloop_Nc_power[sqrvirt=]+output standaloneloop_helas_objects.py:2627,compute_loop_nc=False[None], [0], [None][noborn=]+output madeventgroup_subprocs.py:222,compute_loop_nc=TrueRun 2 genuinely asks for
compute_loop_nc=Trueand genuinely gets a freshmatrix element; it just reads run 1's
Noneback out of the shared memo.get_icolamp_linesis the only consumer of that field (export_v4.py:2132,:2143), which is why the crash is specific to loop-induced madevent output.The same collision is latent within one generation: born strings
(
loop_Nc_power = 0) and closed-fermion-loop strings (= 1) share the memotoo.
The fix
Not
super().__init__(): that gives every matrix element its own memo andloses the cross-ME sharing (measured below).
Not keying the memo on
compute_loop_nceither — that key is still incomplete(it does not separate born from loop strings) and it doubles the memo.
Instead, stop storing
loop_Nc_powerin the memo at all. The colour-algebrasimplification is a pure function of the canonical colour string;
loop_Nc_poweris per-diagram metadata that merely rides along. So the cached copy is stored
with
loop_Nc_power = 0, and on every path the value is stamped back from theincoming string. The cache key stays
canonical_rep, and it is now complete:nothing in the stored value depends on anything else.
This is exactly equivalent on the miss path.
ColorFactor.full_simplifyonlyever
copy.copys a string (ColorString.__copy__ = create_copy, which copiesloop_Nc_power) andproducts another one into it (productdoes not touchloop_Nc_power), so every string it produces already carries the incomingstring's value. Verified by instrumenting
main: overg g > t t~ [virt=QCD]and
u u~ > t t~ [virt=QCD], 51/51 miss-path simplifications produced stringswhose
loop_Nc_powerequalled the input's — zero exceptions.~10 lines in
update_color_basis, plus a per-instance_list_color_dictinLoopColorBasis.__init__(the other piece of parent state it was silentlysharing; always rebound before use, so latent) and a comment recording that
skipping
super().__init__()is deliberate.Byte-identity
Full generated trees, base
28fe3cef1vs branch,PYTHONHASHSEED=0, onemg5_aMCprocess per row. Compared after normalising the two worktree/outputroots; skipping
py3_model.pkl,*.o/.a/.so,me5_configuration.txt,index.htmlandproc_card_mg5.dat(git-describe banner + output path);madevent.tar.gzcompared member by member rather than as a gzip blob.p p > t t~p p > j j(merged particles)g g > t t~ [virt=QCD]g g > h [sqrvirt=QCD]g g > h [noborn=QCD]p p > t t~ [QCD](FKS)Stronger check on the case that actually crashes:
coloramps.incproduced bythe branch's poisoned two-generation run is byte-identical to the one
produced by base's clean single-generation run of the same process. The fix
restores exactly the value the un-poisoned path computes, it does not invent one.
Performance
The memo is what makes multi-subprocess loop generation tractable, so the fix
had to keep it shared.
p p > t t~ j [virt=QCD](4 subprocesses:gu,gu~,uu~,gg),PYTHONHASHSEED=0,update_color_basistimed in-process:28fe3cef1super().__init__()Hit/miss counts and memo size are identical to base — the fix does not
perturb the memo at all, it only changes what is stored in each value. The
colour time difference between base and branch is inside run-to-run noise
(another agent was running madevent jobs on this machine throughout).
The naive per-instance fix, by contrast, loses the cross-matrix-element
sharing: 33% more full simplifications (457 → 608) at this multiplicity,
and the gap grows with the number of subprocesses. That is why this PR does not
take it.
p p > t t~ [QCD]end to end (5 alternating reps per arm, order swappedhalf-way): base 45 / 50 / 24 / 17 / 52 s, branch 35 / 23 / 17 / 16 / 17 s. The
spread is machine contention (another agent was running madevent jobs on this
box); warm steady state is ~17 s on both arms. The in-process colour timing
above is the measurement to trust.
Tests
Both fail on
28fe3cef1, pass here.tests/unit_tests/core/test_color_amp.py::LoopNcMemoTest— drives twoLoopColorBasisobjects through the shared memo with differentloop_Nc_power. On base:AssertionError: [None, None] != [1, 1].A second test pins the memo staying process-wide, so a future
super().__init__()would be caught.tests/acceptance_tests/test_cmd_madloop.py::test_loop_nc_memo_not_poisoned_across_generations— the real reproducer, both generations in one
MasterCmd. On base it errorswith the exact
int - NoneTypetraceback. ~10 s once CutTools is cached.Registered in the existing
unittest.yml/acceptancetest.ymlslots.Full unit suite on the branch: 1491 tests, two non-green results, both
pre-existing and unrelated —
testIO_UnitProcOutputIOTestsis a model-parameterwrite-order golden that needs
PYTHONHASHSEED=0(passes on both arms with itset), and
test_DensityMatrixObservables22errors withModuleNotFoundError: No module named 'scipy'identically on base and branch.Elsewhere in the codebase?
An AST sweep over
madgraph/for "subclass whose__init__skips the parent's,where the parent's
__init__shadows a mutable class attribute" returns exactlyone hit:
LoopColorBasis/_canonical_dict. The pattern is unique to this class.(Unrelated observation, not touched here:
ColorString.complex_conjugaterebuilds the string without
loop_Nc_power, silently resetting it to the classdefault. It is not on any path reached from
update_color_basis.)Note for reviewers
#81 works around this bug — its tests build matrix elements with
compute_loop_nc=Truespecifically to stay clear of the poisoned cache, with acomment saying so. That workaround can be dropped once this lands; this PR does
not touch that branch.
🤖 Generated with Claude Code