Skip to content

No flow afterburner#4113

Merged
pinkenburg merged 2 commits into
sPHENIX-Collaboration:masterfrom
pinkenburg:no-flow-afterburner
Jan 15, 2026
Merged

No flow afterburner#4113
pinkenburg merged 2 commits into
sPHENIX-Collaboration:masterfrom
pinkenburg:no-flow-afterburner

Conversation

@pinkenburg

@pinkenburg pinkenburg commented Jan 15, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Types of changes

  • [ X] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work for users)
  • Requiring change in macros repository (Please provide links to the macros pull request in the last section)
  • I am a member of GitHub organization of sPHENIX Collaboration, EIC, or ECCE (contact Chris Pinkenburg to join)

What kind of change does this PR introduce? (Bug fix, feature, ...)

For OO the flow afterburner is not run, so there are no reaction plane angles. The HeadReco still tried to read those which generated an error at every event.
Also - the PHHepMCGenEventv1 will not return NAN if the flow angle is not set (rather than 0 which is a valid value

TODOs (if applicable)

Links to other PRs in macros and calibration repositories (if applicable)

PR Summary: Flow Afterburner Guard in HeadReco

Motivation / Context

This PR fixes a bug where HeadReco (event header reconstruction module) attempted to read reaction plane (flow) angles unconditionally from HepMC generator events, even when the flow afterburner was not executed in the reconstruction chain. This is a common scenario in "out-of-the-box" (OO) running where optional modules like the flow afterburner may be disabled. The unguarded reads resulted in warnings/errors on every event when flow data was unavailable.

Key Changes

  • HeadReco.cc: Added a guard condition if (!genevt->get_flow_psi_map().empty()) around the loop that sets FlowPsiN(n) values for n=1–6. The loop now only executes if the flow psi map contains data, preventing reads of nonexistent flow angles.

  • PHHepMCGenEventv1.cc: Modified get_flow_psi() to return NaN (std::numeric_limits<float>::quiet_NaN()) instead of 0.0 when a requested flow angle does not exist. This is semantically important because 0.0 is a valid reaction plane angle and would be ambiguous with "not set."

Potential Risk Areas

  • Reconstruction behavior change: Code depending on FlowPsiN values will no longer have them set when flow afterburner is not run. Downstream consumers must be prepared for uninitialized/invalid flow values in such configurations.
  • NaN return value: Shifting from 0.0 to NaN could affect downstream code that uses get_flow_psi() values. Code should explicitly handle NaN cases; numerical comparisons with NaN will always be false, which could cause subtle logic errors if not properly handled.
  • No IO format changes or performance impact anticipated.

Notes

AI-generated summaries should be verified against actual code changes. The modifications are localized and straightforward, but any code consuming FlowPsiN or flow_psi values should be reviewed to ensure proper handling of the new behavior (skipped assignment and NaN returns).

Possible Future Improvements

  • Add comprehensive validation/warning when flow angles are missing across all reconstruction stages
  • Ensure all downstream flow-dependent reconstruction modules explicitly check for NaN or missing flow values before use
  • Consider consolidating flow availability checks to a central location

@coderabbitai

coderabbitai Bot commented Jan 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

These changes modify flow psi value handling in the sPHENIX event reconstruction pipeline. The generator now returns NaN for missing psi_n entries instead of 0.0, and the reconstruction framework adds a guard condition to prevent processing when flow data maps are empty.

Changes

Cohort / File(s) Summary
Flow Psi Return Value Handling
generators/phhepmc/PHHepMCGenEventv1.cc		 Added #include <limits> header. Modified get_flow_psi() to return std::numeric_limits<float>::quiet_NaN() instead of 0.0 when a requested psi_n entry does not exist, with warning logging.
Flow Psi Map Guard Condition
offline/framework/ffamodules/HeadReco.cc		 Added guard check requiring non-empty get_flow_psi_map() before the loop processes FlowPsiN assignments for n=1..6. Prevents unnecessary processing when flow data is absent.
✨ Finishing touches
  • 📝 Generate docstrings
📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bfcf35d and 3d19299.

📒 Files selected for processing (2)
  • generators/phhepmc/PHHepMCGenEventv1.cc
  • offline/framework/ffamodules/HeadReco.cc
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cc,cpp,cxx,c}

⚙️ CodeRabbit configuration file

**/*.{cc,cpp,cxx,c}: Prioritize correctness, memory safety, error handling, and thread-safety.
Flag hidden global state, non-const singletons, and unclear lifetime assumptions.

Only raise Critical or Major findings. Do not post minor style, formatting, naming, or “nice-to-have” refactors.

Files:

  • offline/framework/ffamodules/HeadReco.cc
  • generators/phhepmc/PHHepMCGenEventv1.cc
🧠 Learnings (2)
📚 Learning: 2026-01-10T07:54:57.021Z 
Learnt from: silas-gross
Repo: sPHENIX-Collaboration/coresoftware PR: 4100
File: generators/Herwig/HepMCTrigger/HepMCParticleTrigger.h:93-108
Timestamp: 2026-01-10T07:54:57.021Z
Learning: In HepMCParticleTrigger (generators/Herwig/HepMCTrigger/HepMCParticleTrigger.h), the SetEta* and SetAbsEta* methods intentionally share the same storage variables (_theEtaHigh and _theEtaLow) to emulate the Pythia trigger behavior. This design pattern should be maintained for consistency with existing trigger implementations.

Applied to files:

  • offline/framework/ffamodules/HeadReco.cc
📚 Learning: 2026-01-08T18:39:48.023Z 
Learnt from: emclaughlin2
Repo: sPHENIX-Collaboration/coresoftware PR: 4098
File: offline/QA/Calorimeters/CaloValid.cc:270-270
Timestamp: 2026-01-08T18:39:48.023Z
Learning: In C++ code that parses or traverses data nodes, implement a dual-key fallback when looking up a GL1Packet-like node: first try the version-specific key (e.g., "14001"), then fall back to the legacy/general key (e.g., "GL1Packet"). This ensures compatibility with multiple data versions where the node naming may have changed over time. Apply this pattern to files under the offline/ Calorimeters area (and similar modules) to maintain data-structure resilience across versions.

Applied to files:

  • offline/framework/ffamodules/HeadReco.cc
🔇 Additional comments (2)
generators/phhepmc/PHHepMCGenEventv1.cc (1)

105-114: Returning NaN for missing flow angles is the correct approach.

Since 0.0 is a valid reaction plane angle, using quiet_NaN() as the sentinel for "not set" eliminates ambiguity. Combined with the guard in HeadReco.cc, this warning should only fire if something unexpected happens (e.g., partial map population).

offline/framework/ffamodules/HeadReco.cc (1)

87-93: Correct fix for the missing flow afterburner case.

The guard ensures flow psi values are only read when the afterburner has populated them. If a partial map somehow exists (some n values missing), the get_flow_psi fallback returns NaN with a warning, which is appropriate defensive behavior.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment @coderabbitai help to get the list of available commands and usage tips.

@pinkenburg

pinkenburg commented Jan 15, 2026

Copy link
Copy Markdown
Contributor Author

merge now to make a new ana build for the OO sims

@pinkenburg pinkenburg merged commit 64138d4 into sPHENIX-Collaboration:master Jan 15, 2026
7 of 8 checks passed
@sphenix-jenkins-ci

sphenix-jenkins-ci Bot commented Jan 16, 2026

Copy link
Copy Markdown

Build & test report

Report for commit 3d192990eaa8a96baa4b7b9e604c0287b7dd666d:
Jenkins passed

Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@pinkenburg