Repository navigation
For the INTT hit carry-over issue in streaming, deepen the m_InttRawHitMap size, and discarding non-collision crossings - #4324
Merged
osbornjd merged 4 commits intoJul 2, 2026
Conversation
…t issue in streaming
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds configurable INTT hit duplication and noise-crossing handling in ChangesINTT Hit Duplication and Noise Rejection
Sequence Diagram(s)sequenceDiagram
participant Fun4AllStreamingInputManager
participant FillInttPool
participant m_InttRawHitMap
Fun4AllStreamingInputManager->>FillInttPool: FillIntt()
FillInttPool-->>Fun4AllStreamingInputManager: first successful return
Fun4AllStreamingInputManager->>Fun4AllStreamingInputManager: print INTT state once
Fun4AllStreamingInputManager->>m_InttRawHitMap: iterate hits and rebuild m_InttRawHitCount_FEE
Fun4AllStreamingInputManager->>Fun4AllStreamingInputManager: print verbose GL1-matched state
Possibly related PRs
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. Comment |
Build & test reportReport for commit 807e1cd7807d45508bab079a9ec1c9a95b75ded1:
Automatically generated by sPHENIX Jenkins continuous integration |
ChengWeiShih
marked this pull request as ready for review
June 29, 2026 16:18
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 72bfe51e-800f-482b-9e5c-02f37e28353b
📒 Files selected for processing (3)
offline/framework/fun4allraw/Fun4AllStreamingInputManager.ccoffline/framework/fun4allraw/Fun4AllStreamingInputManager.hoffline/framework/fun4allraw/SingleInttPoolInput.cc
| for (auto bcliter : m_FEEBclkMap) | ||
| { | ||
| if (bcliter.second <= localbclk) | ||
| if (bcliter.second <= localbclk + 120 * 40) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The widened stuck-FEE check now matches FEEs that are ahead of localbclk, so this can erase healthy inputs and drop hits.
bcliter.second <= localbclk + 120 * 40 is more aggressive than the old check: any FEE up to 4800 ticks ahead of localbclk now enters the "stuck" erase path once the pool spread exceeds MaxBclkDiff(). That is the opposite of "allow more slack before erasure" and can discard valid INTT data.
Suggested fix
- if (bcliter.second <= localbclk + 120 * 40)
+ if (bcliter.second <= localbclk &&
+ (localbclk - bcliter.second) >= 120 * 40)As per path instructions, C++ source reviews here should prioritize correctness.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (bcliter.second <= localbclk + 120 * 40) | |
| if (bcliter.second <= localbclk && | |
| (localbclk - bcliter.second) >= 120 * 40) |
Source: Path instructions
Contributor
Author
There was a problem hiding this comment.
No, this wouldn't work, as this is related to how the PacketVector arranges the data.
Contributor
There was a problem hiding this comment.
@ChengWeiShih Thanks for the clarification — since the PacketVector ordering guarantees the relationship between bcliter.second and localbclk at this point in GetSomeMoreEvents, my concern about the widened check matching FEEs ahead of localbclk doesn't apply here. I'll withdraw that suggestion.
✏️ Learnings added
Learnt from: ChengWeiShih
Repo: sPHENIX-Collaboration/coresoftware PR: 4324
File: offline/framework/fun4allraw/SingleInttPoolInput.cc:469-469
Timestamp: 2026-07-02T05:44:54.386Z
Learning: Repo: sPHENIX-Collaboration/coresoftware, File: offline/framework/fun4allraw/SingleInttPoolInput.cc, function SingleInttPoolInput::GetSomeMoreEvents(): The FEE stuck-beamclock check `bcliter.second <= localbclk + 120 * 40` (widened from `bcliter.second <= localbclk`) is correct given how `PacketVector` arranges/orders data; `bcliter.second` values reaching this check do not go ahead of `localbclk` in a way that would erroneously erase valid FEEs. Do not flag this direction-of-comparison concern again for this check.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: pinkenburg
Repo: sPHENIX-Collaboration/coresoftware PR: 4268
File: offline/packages/CaloReco/ClusterCDFCalculator.cc:9-9
Timestamp: 2026-05-18T16:21:14.018Z
Learning: In the sPHENIX core software repository, C++20 std::format is supported (via the <format> header). During code review, do not flag uses of std::format / <format> as a potential C++-version or compiler-compatibility concern anywhere in the C++ codebase. (Other issues may still be flagged as appropriate.)
2 of 5 tasks
Contributor
|
I closed my PR #4290 as discussed in the dNdEta analysis meeting. Below are a few notes for this PR Cheng-Wei can provide the full explanation, but here is my understanding of the differences between the two implementations:
To test 3, Cheng-Wei will compare the INTT doublet distributions as a function of FPHX BCO using two configurations:
|
Contributor
Author
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
offline/framework/fun4allraw/Fun4AllStreamingInputManager.cc (1)
916-918: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCap the carry-over search depth before using it as a loop bound.
InttHitCarryOverShiftMaxMultiple()accepts any non-negativeint, and Line 916 then performs that many map searches for every half-ladder key. A large configuration typo can effectively hang INTT processing. Please bound this setter to the retained strobe depth or a detector-sane maximum before assigningm_InttHitCarryOverShiftMaxMultiple. As per path instructions, prioritize correctness, memory safety, error handling, and thread-safety.Also applies to: 1706-1713
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0cf53601-3fb6-47d5-93cb-0c91c8c32ba5
📒 Files selected for processing (2)
offline/framework/fun4allraw/Fun4AllStreamingInputManager.ccoffline/framework/fun4allraw/Fun4AllStreamingInputManager.h
🚧 Files skipped from review as they are similar to previous changes (1)
- offline/framework/fun4allraw/Fun4AllStreamingInputManager.h
Build & test reportReport for commit a3d221519117c1ab81ac7c913893f2fbc2d26ffa:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit c49adec2d6d6ac74bec64712f97c627acbe01449:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit c32cbc8ea84c7635342f203174f28d9bfa19ef4d:
Automatically generated by sPHENIX Jenkins continuous integration |
2 of 5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
Summary
Key Changes
For recovering missing hits
std::map<uint64_t, InttRawHitInfo> m_InttRawHitMap;Fun4AllStreamingInputManageris not deep enough.m_InttRawHitMapinFun4AllStreamingInputManageris controlled byGetSomeMoreEvents()inSingleInttPoolInput.if (bcliter.second <= localbclk + 120 * 40), ensuring > 40-strobe depth in the map.For the INTT carry-over hit issue in streaming
Public function added in
Fun4AllStreamingInputManager:EnableInttHitDuplication(bool): Enable/disable the mitigation (defaults to disabled)InttHitCarryOverShiftMaxMultiple(int): Set the strobe search depth (defaults to four future strobes)SetIsRejectInttNoiseCrossings(bool): Enable/disable removing INTT hits from non-collision crossings (defaults to disable)SetIsInttStreaming(bool): Set if INTT is in the streaming readout mode (defaults to true)SetIsDuplicateInttFPHXBCOResetHit(bool): enable/disable duplicating INTT hits with FPHXBCO reset to the values defined bystd::vector<int> m_InttResetFphxBcoVec(defaults to true)SetInttResetFphxBcoVec(std::vector<int>): Set thestd::vector<int> m_InttResetFphxBcoVec(default {0 to 5})SetInttStreamingSignalCrossing(std::pair<int,int>): Set the INTT FPHXBCO range with collision,std::pair<int,int> m_InttStreamingSignalCrossing. (defaults to {6, 116}).Implementation in the
Fun4AllStreamingInputManagerfor the INTT carry-over hit issue in the streaming readoutstd::map<std::string, int> m_InttRawHitCount_FEE;, is prepared for every F4A event to count the number of hits for each half-ladder in each crossing of the five strobes in a F4A event (1 GL1BCO)BCOFULL_FPHXBCO_FELIXID_FELIXChannel.m_InttHitCarryOverShiftMaxMultiple.m_IsDuplicateInttFPHXBCOResetHitin theFun4AllStreamingInputManagerto enable/disable duplicating hits with FPHXBCO reset to 0 to 5.For discarding hits from non-collision crossings.
m_IsRejectInttNoiseCrossingsinFun4AllStreamingInputManagerto remove those hits after the carry-over hit correction.TODOs (if applicable)
The current implementation for hit duplication works only with the INTT streaming data. In the near future, we will implement the carry-over hit correction for the INTT-triggered data, which will be incorporated into the current implementation.
Links to other PRs in macros and calibration repositories (if applicable)
Motivation / context:
This PR addresses an INTT streaming readout carry-over issue where hits can be missed across strobes. It extends the effective recovery depth by duplicating INTT raw hits across future BCOs (within a configurable carry-over window) when half-ladder/FPHX association is available. AI-generated summaries can make mistakes—please use best judgment when reading.
Key changes:
Fun4AllStreamingInputManagerto control INTT streaming behavior, including:EnableInttHitDuplication)InttHitCarryOverShiftMaxMultiple, with validation that it must be non-negative)SetIsRejectInttNoiseCrossings,SetIsInttStreaming) using a configurable signal-crossing FPHX_BCO range (SetInttStreamingSignalCrossing)SetIsDuplicateInttFPHXBCOResetHit,SetInttResetFphxBcoVec)Fun4AllStreamingInputManager::FillInttto implement carry-over duplication:m_InttRawHitCount_FEE) keyed by(bco, FPHX_BCO, server/packetid, fee/channel), limited to the active BCO selection windowINTTRAWHIT, optionally skipping hits whoseFPHX_BCOlies outside the configured signal-crossing range (when noise rejection is enabled and INTT streaming is active)m_InttRawHitMapfor future strobes atfuture_bco = base_bco + m_InttHitCarryOverShift * search_i(withsearch_i = 1..m_InttHitCarryOverShiftMaxMultiple), and copies matching hits from the same server/fee/channel when:FPHX_BCOmatches the half-ladder’sFPHX_BCO, orFPHX_BCOis contained inm_InttResetFphxBcoVecbcoandFPHX_BCOoverwritten to the target half-ladder values; noise-crossing rejection is also applied to the half-ladder keys during this duplication passFillIntt:m_Intt_print_count)Verbosity()thresholdsSingleInttPoolInputby widening the acceptable BCL gap (compare againstlocalbclk + 120 * 40instead oflocalbclk).Potential risk areas:
INTTRAWHITdue to added hit duplication and optional exclusion of out-of-range (non-collision) crossings.Fun4AllStreamingInputManagercould be risky if the manager is accessed concurrently without external synchronization.Possible future improvements: