Repository navigation
feat(par-hit): multi-parameter LQ calibration via params config schema - #47
Conversation
run_lq_calibration now loops over config['params'] — one entry per LQ
parameter, each with its own lq_param (no longer hardcoded lq80),
energy_param and optional dt_param/cdf/suffix/plot_options — mirroring
the multi-parameter A/E schema. Results nest per entry under the lq
key, objects as {name: LQCal}, plots per name. Suffixed entries need
LQCal.calibrate suffix support (legend-exp/pygama#707); the kwarg is
only passed when a suffix is configured so unsuffixed entries keep
working with released pygama.
The par-geds-hit-lq entry builds its column list from all entries and
no longer double-nests the full object dict under the pickle's lq key
(it stores just the {name: LQCal} mapping).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ranch Pre-resolves the aoe.py divergence with main: brings the params-loop run_aoe_calibration from #42 into this lineage, combined with the use_log_pdf knob from this branch, and carries the schema fixes from #46 (entry require list, AoE_Uncorr spelling, conditional suffix kwarg). The entry point also stores just the {name: CalAoE} mapping under the pickle's aoe key instead of re-nesting the full object dict, matching the lq script. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR extends the GEDS hit-level LQ and A/E calibrations to support the multi-parameter params config schema (multiple calibrations per run), including optional suffixing to avoid output-name collisions and updated result/object nesting.
Changes:
- Update
run_lq_calibrationandpar-geds-hit-lqto iterate overconfig["params"], supporting per-entrylq_param,energy_param, optionaldt_param/cdf/suffix/plot_options, and storing{name: LQCal}under the pickle’slqkey. - Update
run_aoe_calibrationandpar-geds-hit-aoeto use the sameparamsschema, preserveuse_log_pdf, and nest results/objects/plots per params entry. - Adjust docstrings and CLI-required config keys to match the new schema.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/legenddataflowscripts/par/geds/hit/lq.py | Convert LQ calibration to params-driven multi-calibration flow; update CLI inputs and outputs accordingly. |
| src/legenddataflowscripts/par/geds/hit/aoe.py | Convert A/E calibration to params-driven multi-calibration flow while retaining use_log_pdf; update CLI inputs and outputs accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for name, param_config in configs["params"].items(): | ||
| require_config_keys( | ||
| param_config, | ||
| ["lq_param", "energy_param"], | ||
| f"lq calibration config params entry '{name}'", |
There was a problem hiding this comment.
Agreed — fixed in 197de2a. Added a require_unique_suffixes helper in utils/cfgtools and called it from run_lq_calibration (and run_aoe_calibration) right after the top-level require_config_keys. Two entries resolving to the same suffix — including the two-entries-with-no-suffix case you call out — now fail up front with the duplicated values named (<no suffix> for the None case) rather than silently overwriting LQ_Ecorr/cal-dict keys. Unit-tested in tests/test_utils_helpers.py::test_require_unique_suffixes.
| for name, param_config in config["params"].items(): | ||
| require_config_keys( | ||
| param_config, | ||
| ["current_param", "energy_param"], | ||
| f"aoe calibration config params entry '{name}'", |
There was a problem hiding this comment.
Fixed in 197de2a — same require_unique_suffixes guard is now applied in run_aoe_calibration before any calibration runs, so duplicate (or duplicate-absent) suffixes fail fast instead of silently overwriting AoE_Uncorr/cal-dict entries from earlier iterations. See the sibling comment on lq.py for details.
| dt_cut=param_config.get("dt_cut", None), | ||
| dt_param=param_config.get("dt_param", 3), | ||
| high_cut_val=param_config.get("high_cut_val", 3), | ||
| compt_bands_width=config.get("debug_mode", 20), | ||
| debug_mode=debug_mode | config.get("debug_mode", False), | ||
| **({"use_log_pdf": True} if use_log_pdf else {}), |
There was a problem hiding this comment.
Good catch — that was a genuine bug, not just a naming oddity: with debug_mode set in a config, compt_bands_width received True/False instead of a width. Fixed in 197de2a:
compt_bands_width=param_config.get(
"compt_bands_width", config.get("compt_bands_width", 20)
),per-params-entry with a top-level fallback and the original default of 20. (debug_mode itself is still read separately on the line below, unchanged.)
| for param_config in kwarg_dict["params"].values(): | ||
| params.append(param_config["current_param"]) | ||
| params.append(param_config["energy_param"]) | ||
| if "dt_param" in param_config: | ||
| params.append(param_config["dt_param"]) | ||
| else: | ||
| params.append("dt_eff") | ||
| if "dt_cut" in param_config and param_config["dt_cut"] is not None: | ||
| cal_dict.update(param_config["dt_cut"]["cut"]) | ||
| params.append(param_config["dt_cut"]["out_param"]) | ||
| params = list(dict.fromkeys(params)) |
There was a problem hiding this comment.
Already addressed on this branch by a3dd041 — the CLI validates each entry with require_config_keys(param_config, ["current_param", "energy_param"], f"aoe config params entry '{name}' ({args.config_file})") before indexing them into the column list, so schema errors name the entry and config file consistently with run_aoe_calibration.
Missing current_param/lq_param/energy_param in a params entry now raises a clear error naming the entry instead of a bare KeyError. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… source
Addresses the review comments on the multi-parameter aoe/lq schema:
- new require_unique_suffixes helper (utils/cfgtools): two params entries
resolving to the same output suffix -- including two entries that both
omit it -- silently overwrote each other's derived columns and cal-dict
keys. Both run_aoe_calibration and run_lq_calibration now reject that
up front with a message naming the duplicated suffix values.
- aoe: compt_bands_width was sourced from config.get('debug_mode', 20),
so a config setting debug_mode passed a bool (0/1) as the band width.
Read compt_bands_width explicitly, per-params-entry with a top-level
fallback, defaulting to 20.
- drop the now-unused bare deepcopy import left by the merge resolution.
The two CLI-side comments (validate each params entry before indexing
current_param/energy_param/lq_param) were already addressed on this
branch by a3dd041.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…chema par-geds-pht-aoe, par-geds-pht-lq and the aoe/lq sections of par-geds-pht-fast now read the multi-parameter config schema (top-level cal_energy_param/cut_field plus a params mapping with one entry per PSD parameter), building their DSP column lists from all entries. Matches the multi-parameter run_aoe_calibration/run_lq_calibration in legend-dataflow-scripts (legend-exp/legend-dataflow-scripts#47). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…chema par-geds-pht-aoe, par-geds-pht-lq and the aoe/lq sections of par-geds-pht-fast now read the multi-parameter config schema (top-level cal_energy_param/cut_field plus a params mapping with one entry per PSD parameter), building their DSP column lists from all entries. Matches the multi-parameter run_aoe_calibration/run_lq_calibration in legend-dataflow-scripts (legend-exp/legend-dataflow-scripts#47). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r params schema (#164) * feat(par-pht): migrate aoe/lq scripts to the multi-parameter params schema par-geds-pht-aoe, par-geds-pht-lq and the aoe/lq sections of par-geds-pht-fast now read the multi-parameter config schema (top-level cal_energy_param/cut_field plus a params mapping with one entry per PSD parameter), building their DSP column lists from all entries. Matches the multi-parameter run_aoe_calibration/run_lq_calibration in legend-dataflow-scripts (legend-exp/legend-dataflow-scripts#47). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * review: validate params entries up front in the pht scripts Missing current_param/lq_param/energy_param in a params entry now raises a clear error naming the channel and entry instead of a bare KeyError. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * review: validate dt_cut out_param in aoe params entries A dt_cut mapping missing out_param now raises a clear error naming the channel and params entry instead of a bare KeyError. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(par-psp): validate requested peaks against the union of peak files Mirror the par-dsp dplms check in the psp variant: a peak may legitimately be absent from an individual run's file, so validate the dplms config's requested peaks against the union over all peak files with require_peaks_present before reading waveforms, instead of silently selecting zero events. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Summary
Extends the LQ calibration to handle multiple LQ parameters, mirroring the multi-parameter A/E schema from #42:
run_lq_calibrationloops overconfig["params"]— one entry per LQ parameter, each with its ownlq_param(no longer hardcodedlq80),energy_param, and optionaldt_param/cdf/suffix/plot_options. Results nest per entry under thelqkey, pickled objects as{name: LQCal}, plots per name. Config shape:Suffixed entries require
LQCal.calibratesuffix support (legend-exp/pygama#707); the kwarg is only passed when a suffix is configured, so unsuffixed entries keep working with released pygama. Thepar-geds-hit-lqentry point builds its column list from all entries and now stores just the{name: LQCal}mapping under the pickle'slqkey instead of re-nesting the full object dict.The second commit pre-resolves the
aoe.pydivergence with main: it brings the params-looprun_aoe_calibrationfrom #42 into this lineage combined with theuse_log_pdfknob, and carries the schema fixes from #46.Stacked on #44 (
perf-par-dsp) — only the two commits on top are new.Test plan
Full suite (unit + skimmed-data integration hit chain, ecal→aoe→qc→lq): 71 passed locally, with the LQ config exercising an unsuffixed primary plus a suffixed duplicate entry end to end, and the aoe config migrated to the params schema. Companion legend-dataflow pht chain (partcal→aoe→lq→fast) also passes.
Per
AI_POLICY.md: developed with AI assistance (Claude); reviewed by the submitter.🤖 Generated with Claude Code