Add a function to universally mask the INTT chip#4225
Conversation
📝 WalkthroughWalkthroughThe pull request adds an early filtering mechanism to the INTT combined raw data decoder to skip permanently bad chips during hit processing. A new private data member stores chip identifiers marked as permanently bad, and processing logic checks this list before the bco filter stage. Changes
📝 Coding Plan
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 Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d99107e6-d282-403d-973a-6ee1003cfc5d
📒 Files selected for processing (2)
offline/packages/intt/InttCombinedRawDataDecoder.ccoffline/packages/intt/InttCombinedRawDataDecoder.h
| if (std::find(permanant_mask_chip.begin(), permanant_mask_chip.end(), std::format("{}_{}_{}", raw.felix_server, raw.felix_channel, raw.chip)) != permanant_mask_chip.end()) | ||
| { | ||
| if (1 < Verbosity()) | ||
| { | ||
| std::cout | ||
| << PHWHERE << "\n" | ||
| << "\tMasking permanant bad chip due to timing issues:\n" | ||
| << "\t" << raw.felix_server << " " << raw.felix_channel << " " << raw.chip << " " << raw.channel << "\n" | ||
| << std::endl; | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Chip-ID convention is ambiguous here and can permanently mask the wrong chip.
Line 354 matches on raw.chip (normalized mapping path), while the configured identifier is human-labeled as “Chip15”. Without an explicit, single normalization contract, an off-by-one convention mismatch will silently filter the wrong hardware chip for all events.
Please centralize chip-key normalization in one helper (used for both configuration and runtime comparison) and add a regression test that proves the intended mapping for FELIX2_FELIXChannel9_Chip15.
As per coding guidelines, **/*.{cc,cpp,cxx,c}: “Prioritize correctness, memory safety, error handling, and thread-safety.”
There was a problem hiding this comment.
What is the fix suggestion?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the definition of RawFromHit in InttNameSpace
rg -n "RawFromHit" --type=cc -C5
rg -n "RawFromHit" --type=h -C10
Length of output: 1702
🏁 Script executed:
#!/bin/bash
# Find RawData_s struct definition and RawFromHit implementation
rg -n "struct RawData_s" --type=h -C10
rg -n "RawFromHit" -C10 --glob="*.cc" --glob="*.cpp" --glob="*.cxx"
Length of output: 7268
@ChengWeiShih — here is the concrete issue and fix.
Root Cause
InttNameSpace::RawFromHit normalizes the chip ID as:
// offline/packages/intt/InttMapping.cc, line 191
.chip = (hit->get_chip_id() + 25) % 26, // converts 1-indexed → 0-indexedSo hardware Chip15 (1-indexed label) → get_chip_id() = 15 → raw.chip = (15 + 25) % 26 = **14**.
But the default mask entry in the header is "2_9_15" — which will never match raw.chip = 14 at runtime. The mask silently does nothing.
Minimal Fix
Change the default entry in InttCombinedRawDataDecoder.h from "2_9_15" to "2_9_14", and add a comment explaining the convention:
// Chip indices use the raw.chip convention: (get_chip_id() + 25) % 26 (0-based).
// Hardware label "Chip15" (1-based) → raw.chip = 14.
std::vector<std::string> permanant_mask_chip = {"2_9_14"};Recommended Fix (prevents future recurrence)
Add a small static helper that encodes the conversion once and is used both when building entries and when comparing at runtime:
// InttCombinedRawDataDecoder.h
static std::string MakeChipKey(int felix_server, int felix_channel, int chip_label_1based)
{
// raw.chip convention: (chip_id + 25) % 26 — mirrors InttNameSpace::RawFromHit
return std::format("{}_{}_{}", felix_server, felix_channel,
(chip_label_1based + 25) % 26);
}
// Initialize using the helper so the intent is self-documenting:
// FELIX2 / Channel9 / Chip15 (1-based hardware label) → key "2_9_14"
std::vector<std::string> permanant_mask_chip = {MakeChipKey(2, 9, 15)};And in InttCombinedRawDataDecoder.cc, the runtime comparison already uses raw.chip directly which is already 0-indexed — so no change is needed there. The helper only needs to be applied to the configuration side.
This way anyone adding a new chip to mask passes the human-readable hardware label and the arithmetic error is impossible.
There was a problem hiding this comment.
@ChengWeiShih did you explicitly test this to see if the correct chip is masked?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@osbornjd Yes, I have tested. I am pretty sure this is the correct chip to be masked.
There was a problem hiding this comment.
I trust you over the AI 😁
| std::vector<std::string> permanant_mask_chip = { | ||
| "2_9_15" // note : FELIX 2, FELIX channel 9, chip 15 (chip ID range: 0 to 25) | ||
| }; |
There was a problem hiding this comment.
Hard-coded permanent masking has no runtime opt-out and can silently drop data across all productions.
Line 98 bakes "2_9_15" into decoder state with no public control to disable/override it per job. That makes reconstruction behavior non-configurable for MC, reprocessing, or future detector-status changes, and can break comparability across productions.
Proposed API-level fix
class InttCombinedRawDataDecoder : public SubsysReco
{
public:
+ void clearPermanentMaskChips();
+ void addPermanentMaskChip(int felix_server, int felix_channel, int chip);
+ void useDefaultPermanentMask(bool enable = true);
private:
- std::vector<std::string> permanant_mask_chip = {
- "2_9_15"
- };
+ std::vector<std::string> permanant_mask_chip;
};As per coding guidelines, **/*.{h,hpp,hxx,hh}: “Focus on API clarity/stability… If interfaces change, ask for compatibility notes and any needed downstream updates.”
Build & test reportReport for commit 54589d3163e44d4f5ed574bed5989546b28a1989:
Automatically generated by sPHENIX Jenkins continuous integration |
2 participants
comment:
Based on our study, currently, there is one bad chip that cannot be detected by our bad channel classifier. And this chip has been bad throughout the sPHENIX data taking. But it's painful to update all the INTT_HotMap in CDB. Therefore, we would like to permanently mask this chip directly in this module.
Detail: The chip, FELIX2_FELIXChannel9_Chip15, the total number of hits looks reasonable, but the hits timing has a shift, which is bad. And our bad channel classifier doesn't check the timing (because of the extended/streaming readout). So this chip remains unmasked.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
comment: Add a function to permanently mask an INTT chip.
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
Motivation / Context
One INTT chip (FELIX2_FELIXChannel9_Chip15) has been persistently problematic throughout sPHENIX data taking. While its total hit counts appear reasonable, the hit timing distribution shows a systematic shift. The existing bad-channel classifier does not detect this issue because it does not evaluate timing information, which is unavailable due to extended/streaming readout mode. This PR introduces a mechanism to permanently mask such problematic chips at the decoder level rather than managing entries in the INTT_HotMap CDB.
Key Changes
Header (.h): Added a private data member
permanant_mask_chipas astd::vector<std::string>initialized with one entry:"2_9_15"(representing FELIX server, FELIX channel, and chip ID in underscore-delimited format).Implementation (.cc): Added an early chip-level filter in the raw data decoding loop that checks if the current hit's chip matches an entry in the permanent mask list. When a match is found, the hit is skipped with an optional verbose log message (printed when Verbosity() > 1). The check is positioned after per-channel bad-map filtering but before BCO timing validation.
Filtering Order: The permanent chip mask operates in the following sequence:
Potential Risk Areas
Reconstruction Behavior: This change directly drops all hits from masked chips, potentially reducing tracking efficiency for events with tracks crossing the masked chip region. The impact depends on the chip's detector coverage and track densities in sPHENIX runs.
Hardcoded Configuration: The mask list is hardcoded as a default member initializer. Future chips requiring permanent masking would require code changes rather than configuration-based updates. Consider whether a configuration file or CDB-based approach would be more maintainable.
String Matching Performance: The implementation uses
std::find()with string comparisons. For a single masked chip this is negligible, but performance could be a consideration if the mask list grows substantially.Verbosity Output: Debug logging to stdout may be voluminous in high-rate data processing when Verbosity() > 1 is enabled.
Possible Future Improvements
Note: AI tools can make mistakes. Verify the formatting of the chip identifier strings and confirm that the selected chip (2_9_15) is indeed the problematic one discussed in detector operations.