fix(par-hit): align aoe entry checks and naming with the params schema - #46
Open
ggmarshall wants to merge 2 commits into
Open
fix(par-hit): align aoe entry checks and naming with the params schema#46ggmarshall wants to merge 2 commits into
ggmarshall wants to merge 2 commits into
Conversation
Three follow-ups to the multi-parameter A/E config schema (#42): - par-geds-hit-aoe still required top-level current_param/energy_param, which no longer exist in params-style configs; require cal_energy_param/cut_field/threshold/params instead (the per-entry keys are read inside the params loop). - restore the historical AoE_Uncorr spelling for the unsuffixed initial parameter (the params rewrite introduced lowercase AoE_uncorr, renaming the hit-level operation). - pass the suffix kwarg to CalAoE.calibrate only when a suffix is configured, so unsuffixed configs keep working with released pygama versions that predate suffix support. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR applies follow-up fixes to the par-geds-hit-aoe A/E calibration step after the multi-parameter params schema change, ensuring migrated configs validate correctly and output parameter naming remains consistent with historical conventions.
Changes:
- Update A/E config validation to require
params(and other schema-aligned top-level keys) instead of legacy top-levelcurrent_param/energy_param. - Restore the historical unsuffixed uncorrected parameter name
AoE_Uncorr(vsAoE_uncorr). - Only pass
suffixintoCalAoE.calibrate()when a suffix is actually configured, preserving compatibility with older pygama releases.
Comments suppressed due to low confidence (1)
src/legenddataflowscripts/par/geds/hit/aoe.py:393
- The top-level config keys are now validated, but each entry in
kwarg_dict["params"]is still accessed viaparam_config["current_param"]/param_config["energy_param"]without validation. A missing per-entry key will crash withKeyErrorinstead of producing the intended user-facing config error; validating each entry here will make failures actionable.
require_config_keys(
kwarg_dict,
["cal_energy_param", "cut_field", "threshold", "params"],
f"aoe config ({args.config_file})",
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
run_aoe_calibration now requires cal_energy_param/cut_field/params up front and each params entry to carry current_param and energy_param, raising a clear error naming the offending entry; same for the entry-point column-gathering loop. Co-Authored-By: Claude Fable 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.
Three small follow-ups to the multi-parameter A/E config schema merged in #42:
par-geds-hit-aoestill required top-levelcurrent_param/energy_param, which no longer exist inparams-style configs — any migrated config failed the up-front check. It now requirescal_energy_param/cut_field/threshold/params(the per-entry keys are read inside the params loop).AoE_Uncorrspelling for the unsuffixed initial parameter — the params rewrite introduced lowercaseAoE_uncorr, silently renaming the hit-level operation in output par files.suffixkwarg toCalAoE.calibrateonly when a suffix is actually configured, so unsuffixed configs keep working with released pygama versions that predate suffix support (pargen: suffix support in LQCal for calibrating multiple LQ parameters pygama#707 / theaoebranch).The
run_aoe_calibrationdocstring is updated to describe theparamsschema.Per
AI_POLICY.md: developed with AI assistance (Claude); reviewed by the submitter.🤖 Generated with Claude Code