Skip to content

fix(par-hit): align aoe entry checks and naming with the params schema - #46

Open
ggmarshall wants to merge 2 commits into
mainfrom
aoe-params-schema-fixes
Open

fix(par-hit): align aoe entry checks and naming with the params schema#46
ggmarshall wants to merge 2 commits into
mainfrom
aoe-params-schema-fixes

Conversation

@ggmarshall

Copy link
Copy Markdown
Contributor

Three small follow-ups to the multi-parameter A/E config schema merged in #42:

  • par-geds-hit-aoe still required top-level current_param/energy_param, which no longer exist in params-style configs — any migrated config failed the up-front check. It now requires cal_energy_param/cut_field/threshold/params (the per-entry keys are read inside the params loop).
  • Restores the historical AoE_Uncorr spelling for the unsuffixed initial parameter — the params rewrite introduced lowercase AoE_uncorr, silently renaming the hit-level operation in output par files.
  • Passes the suffix kwarg to CalAoE.calibrate only 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 / the aoe branch).

The run_aoe_calibration docstring is updated to describe the params schema.

Per AI_POLICY.md: developed with AI assistance (Claude); reviewed by the submitter.

🤖 Generated with Claude Code

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>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-level current_param/energy_param.
  • Restore the historical unsuffixed uncorrected parameter name AoE_Uncorr (vs AoE_uncorr).
  • Only pass suffix into CalAoE.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 via param_config["current_param"] / param_config["energy_param"] without validation. A missing per-entry key will crash with KeyError instead 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.

Comment thread src/legenddataflowscripts/par/geds/hit/aoe.py
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>
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.

2 participants