Skip to content

fix Zlm static user vars flag - #413

Merged
kevScheuer merged 3 commits into
masterfrom
shepherd_amp_fix
Sep 3, 2026
Merged

fix Zlm static user vars flag#413
kevScheuer merged 3 commits into
masterfrom
shepherd_amp_fix

Conversation

@mashephe

@mashephe mashephe commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

This corrects the logic in the Zlm class for "areUserVarsStatic"? This has implications on how AmpTools optimizes the storage of user variables. The current implementation would lead to incorrect fit results when the same accepted MC is used to fit all polarization orientations, and the polarization angle and magnitude is passed as an argument to the class.

I also deleted an unused header from Vec_ps_refl to enhance portability.

Matthew Shepherd added 2 commits September 2, 2026 16:40
…the polarization is stored in the data file or not - this generates the correct behavior for the current implementation of the class
Copilot AI lite review requested due to automatic review settings September 2, 2026 20:49

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.

🟢 Approval recommended

The functional change aligns areUserVarsStatic() with how Zlm actually computes user variables, and the remaining feedback is a minor comment typo.

Pull request overview

Adjusts AmpTools’ optimization behavior for the Zlm amplitude by correctly reporting when its computed user variables can be shared across amplitude instances, preventing incorrect fits when polarization is provided via amplitude arguments rather than embedded in event data. Also removes an unused include to improve portability.

Changes:

  • Make Zlm::areUserVarsStatic() depend on whether polarization is stored in the input tree (m_polInTree) instead of always returning true.
  • Remove unused UTILITIES/BeamProperties.h include from Vec_ps_refl.cc.
File summaries
File Description
src/libraries/AMPTOOLS_AMPS/Zlm.h Correctly reports whether user variables are static across amplitude instances based on polarization source.
src/libraries/AMPTOOLS_AMPS/Vec_ps_refl.cc Drops an unused header include to reduce unnecessary dependencies.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/libraries/AMPTOOLS_AMPS/Zlm.h Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@gluex

gluex commented Sep 2, 2026

Copy link
Copy Markdown

Test status for this pull request: SUCCESS

Summary: summary.txt
Logs: results/log

Build log: make_shepherd_amp_fix.log
Build report: report_shepherd_amp_fix.txt

@gluex

gluex commented Sep 2, 2026

Copy link
Copy Markdown

Test status for this pull request: SUCCESS

Summary: summary.txt
Logs: results/log

Build log: make_shepherd_amp_fix.log
Build report: report_shepherd_amp_fix.txt

@kevScheuer kevScheuer 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.

Good catch

@kevScheuer
kevScheuer merged commit 15e7989 into master Sep 3, 2026
2 checks passed
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.

4 participants