Skip to content

fix missing f prefix in the trigger keep_columns comprehension - #156

Open
artlbv wants to merge 1 commit into
uhh-cms:HH_HHH_incl_2024from
artlbv:fix/trigger-keep-columns-fstring
Open

artlbv wants to merge 1 commit into
uhh-cms:HH_HHH_incl_2024from
artlbv:fix/trigger-keep-columns-fstring

Conversation

@artlbv

@artlbv artlbv commented Sep 8, 2026

Copy link
Copy Markdown

Fixes #155.

The bug

hbw/config/config_run2.py:1397 builds the per-trigger HLT entries of
keep_columns["cf.ReduceEvents"] with a set comprehension that is not an f-string:

} | {
    "HLT.{trg.hlt_field}" for trg in cfg.get_aux("triggers", [])
}

So it adds the literal string "HLT.{trg.hlt_field}" exactly once, rather than one entry per
registered Trigger. One character.

Impact — smaller than I first claimed in #155

I originally wrote that this broke what ComputeTriggerSF asks for. That was wrong, and I want
to correct it here rather than leave it in the issue:

  • hbw_trigger_selection's init (hbw/selection/trigger.py:188-192) adds the same set,
    correctly f-stringed
    , to keep_columns["cf.ReduceEvents"]. Any selector routing through it
    already keeps its registered triggers' HLT columns.
  • Downstream code reads trigger_ids — built in the selector at trigger.py:52-134, kept by
    name at config_run2.py:1388 — not the HLT path columns. require_triggers,
    masks_dih.py, and hbw/production/trigger.py are all id-based.

So this is latent, not active, and the DL analysis is unaffected. Thanks to @mafrahm for the
trigger_ids point, which is what prompted the recheck.

It bites in exactly one place: a selector that does not route through hbw_trigger_selection
and still wants a registered trigger's raw HLT column. That is the SL channel, which uses the
year ladder in sl_remastered.py — I hit it needing
HLT.Mu12_IsoVVL_PFHT150_PNetBTag0p53 to survive ReduceEvents for a trigger study.

Why fix it anyway

The line does not do what it reads as doing, and the working duplicate in trigger.py is
labelled # testing:, so it is not obviously the load-bearing one. Leaving a silently inert
comprehension next to a hand-maintained list of seven HLT paths is a trap for whoever next
assumes registering a Trigger is sufficient to keep its column — which is what the code
appears to promise.

Worth a separate look (not in this PR): the two mechanisms are redundant, and the # testing:
block in trigger.py is arguably the one that should go once this one works.

Change

The f prefix, plus a comment recording that the effect was masked so the next reader does not
have to re-derive it. No behaviour change for DL: the columns it adds are already added by
trigger.py.

tests/run_linting clean.

The set comprehension building the per-trigger HLT entries of
keep_columns["cf.ReduceEvents"] was not an f-string, so it added the
literal string "HLT.{trg.hlt_field}" once instead of one entry per
registered trigger.

The effect is currently masked: hbw_trigger_selection's init adds the
same set correctly, and downstream code reads trigger_ids rather than
the HLT path columns. So this is latent rather than active -- it only
bites a selector that does not route through hbw_trigger_selection and
still wants a registered trigger's raw HLT column.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MdTWv3sC68nLDdYw12GV7K
@artlbv

artlbv commented Sep 8, 2026

Copy link
Copy Markdown
Author

@mafrahm fyi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant