Skip to content

Calopacketcontainer ffarawmodules - #4508

Merged
pinkenburg merged 2 commits into
sPHENIX-Collaboration:masterfrom
pinkenburg:calopacketcontainer-ffarawmodules
Oct 28, 2026
Merged

pinkenburg merged 2 commits into
sPHENIX-Collaboration:masterfrom
pinkenburg:calopacketcontainer-ffarawmodules

Conversation

@pinkenburg

@pinkenburg pinkenburg commented Oct 27, 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 CaloPacketContainer from framework/ffarawmodules, Modifying the ClockDiffCheck and removing obsolete modules which did not use the new CaloPacket class

TODOs (if applicable)

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

Motivation / context

This change removes CaloPacketContainer-based modules from framework/ffarawmodules as the codebase moves toward individual CaloPacket nodes. The supplied description marks the change as a new feature.

Key changes

  • ClockDiffCheck now checks individual CaloPacket nodes and no longer declares the container-level FillCaloClockDiff method.
  • When delBadPkts is enabled, ClockDiffCheck resets packets with XMIT clock mismatches. It also resets packets that fail CheckFemEventNr.
  • Removed the CemcCheck, HcalCheck, MbdCheck, SEpdCheck, and ZdcCheck modules and their headers.
  • Updated Makefile.am to remove those modules and add BcoRangeCheck to the build lists.

Potential risk areas

  • Reconstruction behavior: Packet reset conditions may change downstream processing. FEM event-number failures reset packets regardless of delBadPkts.
  • Compatibility: Removing the check modules can affect workflows that still instantiate them.
  • IO format: No IO format change is established by the supplied evidence.
  • Thread-safety and performance: No findings were supplied for these areas.

Possible future improvements

  • Confirm that workflows have replacements for the removed check modules.
  • Add or update tests for XMIT mismatch handling, FEM event-number failures, and configurations with delBadPkts enabled or disabled.
  • Document migration steps for users of the removed modules.

AI-generated change descriptions can contain errors. Use the source and project knowledge to verify details. Test results were not supplied.

@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Walkthrough

ClockDiffCheck now checks individual CaloPacket nodes and resets packets that fail its checks. The PR also removes five detector-specific packet-check modules and updates the ffarawmodules build lists.

Changes: Packet-check modules

Layer / File(s) Summary
Check individual CaloPacket nodes
offline/framework/ffarawmodules/ClockDiffCheck.cc, offline/framework/ffarawmodules/ClockDiffCheck.h
ClockDiffCheck replaces combined-container processing with individual CaloPacket checks. It resets XMIT clock mismatches when delBadPkts is enabled, and resets packets that fail CheckFemEventNr regardless of that flag. The container helper and its declaration are removed.
Remove detector-specific check modules
offline/framework/ffarawmodules/{CemcCheck,HcalCheck,MbdCheck,SEpdCheck,ZdcCheck}.{cc,h}, offline/framework/ffarawmodules/Makefile.am
The five detector-specific packet-check classes and implementations are deleted. The build lists add BcoRangeCheck.h and remove the deleted headers and source files.

Possibly related PRs

Priority: Low

Merge Risk: Moderate · up to 99088

Inconsistent FEM packets can remain in detector data when bad-packet removal is enabled; restore detector-wide filtering 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.

Caution

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

⚠️ Outside diff range comments (1)

Major · Per-packet FEM event check cannot catch an outlier packet within a… · ClockDiffCheck.cc:183-220

offline/framework/ffarawmodules/ClockDiffCheck.cc:183-220
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Per-packet FEM event check cannot catch an outlier packet within a detector.

At ClockDiffCheck::process_event, CheckFemEventNr(calopacket) runs on one CaloPacket node at a time. A packet that holds one FEM module gives the helper a one-element event-number set, so the check always passes. If that packet's FEM event number differs from the other packets of the same detector, it stays in the output even when delBadPkts is enabled.

The removed container logic selected the majority FEMEVTNR across the packets of each detector and removed mismatching packets when delBadPkts was set. Restore detector-wide majority selection before the per-node loop. This requires a packet-to-detector grouping that the inspected code does not provide. Per-node checking does not replace it.


Review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 911a978 and 9908833.

Files selected for processing: 13
  • offline/framework/ffarawmodules/CemcCheck.cc
  • offline/framework/ffarawmodules/CemcCheck.h
  • offline/framework/ffarawmodules/ClockDiffCheck.cc
  • offline/framework/ffarawmodules/ClockDiffCheck.h
  • offline/framework/ffarawmodules/HcalCheck.cc
  • offline/framework/ffarawmodules/HcalCheck.h
  • offline/framework/ffarawmodules/Makefile.am
  • offline/framework/ffarawmodules/MbdCheck.cc
  • offline/framework/ffarawmodules/MbdCheck.h
  • offline/framework/ffarawmodules/SEpdCheck.cc
  • offline/framework/ffarawmodules/SEpdCheck.h
  • offline/framework/ffarawmodules/ZdcCheck.cc
  • offline/framework/ffarawmodules/ZdcCheck.h
Files with no reviewable changes: 12
  • offline/framework/ffarawmodules/MbdCheck.h
  • offline/framework/ffarawmodules/ZdcCheck.h
  • offline/framework/ffarawmodules/ClockDiffCheck.h
  • offline/framework/ffarawmodules/SEpdCheck.cc
  • offline/framework/ffarawmodules/SEpdCheck.h
  • offline/framework/ffarawmodules/HcalCheck.h
  • offline/framework/ffarawmodules/CemcCheck.h
  • offline/framework/ffarawmodules/HcalCheck.cc
  • offline/framework/ffarawmodules/MbdCheck.cc
  • offline/framework/ffarawmodules/CemcCheck.cc
  • offline/framework/ffarawmodules/ZdcCheck.cc
  • offline/framework/ffarawmodules/ClockDiffCheck.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.

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 9908833ee4850920c57858c8ef42a0b0d30310c0:
Jenkins passed


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@pinkenburg
pinkenburg merged commit 601b7ea into sPHENIX-Collaboration:master Oct 28, 2026
22 checks passed
@pinkenburg
pinkenburg deleted the calopacketcontainer-ffarawmodules branch October 28, 2026 10:29
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