Skip to content

Simplify calotowerbuilder - #4499

Merged
pinkenburg merged 5 commits into
sPHENIX-Collaboration:masterfrom
pinkenburg:simplify-calotowerbuilder
Oct 25, 2026
Merged

pinkenburg merged 5 commits into
sPHENIX-Collaboration:masterfrom
pinkenburg:simplify-calotowerbuilder

Conversation

@pinkenburg

@pinkenburg pinkenburg commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • [ X] 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, ...)

This PR removes the used of the long obsolete (years) CaloPacketContainer object and the need to set set_offlineflag(true) in the macro. The CaloTowerBuilder now checks the nodes (Event or CaloPacket) and calls the appropriate reconstruction rather than relying on the user to figure this out. Also moved the lambda function to the beginning of the method for clarity rather than dumping it somewhere in the middle.

TODOs (if applicable)

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

Motivation

Simplify CaloTowerBuilder input handling. Remove reliance on the obsolete CaloPacketContainer and on set_offlineflag to select an input path.

Key changes

  • process_data uses packets from the PRDF Event when that node exists.
  • If the event type is not DATAEVENT, the builder returns ABORTEVENT.
  • If no PRDF Event exists, the builder looks up individual CaloPacket nodes by packet ID.
  • Keep set_offlineflag for compatibility, but its argument no longer controls input selection.
  • Remove obsolete packet-selection state and update a calibration diagnostic in TpcCombinedRawDataUnpacker.

Potential risk areas

  • If both input forms are present, the builder uses the PRDF Event; it does not read the individual CaloPacket nodes.
  • If a packet is missing, the builder fills its channels with -1 waveforms. Validate this behavior for the workflows that use packet nodes.
  • No test results were provided.

Possible future improvements

  • Document the input precedence and missing-packet behavior.
  • Clarify whether set_offlineflag will remain for API compatibility.
  • Add regression tests for both input paths, event-type handling, and missing packets.

AI-generated summaries can make mistakes. Verify these details against the implementation and relevant reconstruction workflows. Review-finding counts are unavailable from the supplied information.

@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 1b6fe02 and fedd197.

Files selected for processing: 2
  • offline/packages/CaloReco/CaloTowerBuilder.cc
  • offline/packages/CaloReco/CaloTowerBuilder.h
Files skipped from review as they are similar to previous changes: 2
  • offline/packages/CaloReco/CaloTowerBuilder.h
  • offline/packages/CaloReco/CaloTowerBuilder.cc

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

CaloTowerBuilder now processes packets only from a present PRDF event and removes offline packet input paths. The TPC unpacker’s missing-calibration diagnostic now names TpcCombinedRawDataUnpacker::InitRun; its exit behavior is unchanged.

Changes: Calorimeter PRDF packet processing

Layer / File(s) Summary
PRDF packet processing
offline/packages/CaloReco/CaloTowerBuilder.h, offline/packages/CaloReco/CaloTowerBuilder.cc
process_data processes packets from a present PRDF event. It aborts for a present non-data event and skips packet processing when no PRDF event exists. The offline packet paths and packet-flag members are removed. set_offlineflag remains but stores its argument in tmpdummy.

TPC calibration diagnostic

Layer / File(s) Summary
Calibration diagnostic message
offline/packages/tpc/TpcCombinedRawDataUnpacker.cc
The missing-calibration message now names TpcCombinedRawDataUnpacker::InitRun. The subsequent exit(1) is unchanged.

Priority: Normal

Merge Risk: Moderate · up to fedd1

Events without calorimeter input can acquire synthetic towers. Skip tower generation for those events before merging.

- [ ] Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1


Review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between bfce189 and 128874d.

Files selected for processing: 3
  • offline/packages/CaloReco/CaloTowerBuilder.cc
  • offline/packages/CaloReco/CaloTowerBuilder.h
  • offline/packages/tpc/TpcCombinedRawDataUnpacker.cc

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Comment thread offline/packages/CaloReco/CaloTowerBuilder.cc
@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 128874d4799db69d8a15640d8f786305ba4c00f5:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@coderabbitai coderabbitai Bot 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.

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

Minor · Propagate malformed CaloPacket failures. · CaloTowerBuilder.cc:421-425

offline/packages/CaloReco/CaloTowerBuilder.cc:421-425
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Propagate malformed CaloPacket failures.

When CaloPacket::iValue(0, "CHANNELS") exceeds m_nchannels, process_packet returns ABORTEVENT. This fallback discards that result, so process_data can return EVENT_OK and process_event can accept the event with missing or partial tower reconstruction. The base code had the same omission in its explicit offline-node branch, but the PR makes this path active by default when PRDF is absent.

Suggested fix
       CaloPacket *calopacket = findNode::getClass<CaloPacket>(topNode, pid);
-      process_packet(calopacket, pid);
+      if (process_packet(calopacket, pid) == Fun4AllReturnCodes::ABORTEVENT)
+      {
+        return Fun4AllReturnCodes::ABORTEVENT;
+      }

Review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 128874d and 1b6fe02.

Files selected for processing: 1
  • offline/packages/CaloReco/CaloTowerBuilder.h

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 1b6fe028ad7ca83833ff5a523558306b11ccc38a:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit fedd19734688718b5808bfa6d7d9d010c670cb10:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@pinkenburg
pinkenburg merged commit 5ef5c4a into sPHENIX-Collaboration:master Oct 25, 2026
18 of 22 checks passed
@pinkenburg
pinkenburg deleted the simplify-calotowerbuilder branch October 25, 2026 22:41
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.

1 participant