[PWGCF] fix-ft0-gain-indexing-v3#14496
Conversation
- Use channel ID instead of array index for gain correction - Fixes mismatched amplitude corrections for remapped dead channels
- Use channel ID instead of array index for FT0 gain correction - Preserve original amplitude values for dead channel mapping - Resolve merge conflict markers
Preserve original amplitude values when filling dead channel histograms Properly declare mirroredAmpl variables
MartijnLaarhoven
requested review from
alibuild,
ariedel-cern,
glromane,
lauraser,
lgraczykCern,
otonvd,
prchakra,
shouqiye,
victor-gonzalez and
zchochul
as code owners
github-actions
bot
changed the title
|
O2 linter results: ❌ 0 errors, |
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug in the FT0 (Fast Interaction Trigger) channel amplitude handling for dead channel remapping in dihadron correlation analysis. The bug caused incorrect amplitude values to be recorded in histograms due to premature modification of the amplitude variable.
Changes:
- Fixed amplitude variable corruption when filling dead channel histograms by introducing separate variables (
mirroredAmplandmirroredAmplCorrected) to preserve original amplitude values - Added early filtering of rejected FT0 channels in the correlation loop to avoid unnecessary processing
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (corType == kFT0C) { | ||
| if ((cfgRejectFT0CInside && (chanelid >= kFT0CInnerRingMin && chanelid <= kFT0CInnerRingMax)) || (cfgRejectFT0COutside && (chanelid >= kFT0COuterRingMin && chanelid <= kFT0COuterRingMax))) | ||
| continue; | ||
| } else if (corType == kFT0A) { | ||
| if ((cfgRejectFT0AInside && (chanelid >= kFT0AInnerRingMin && chanelid <= kFT0AInnerRingMax)) || (cfgRejectFT0AOutside && (chanelid >= kFT0AOuterRingMin && chanelid <= kFT0AOuterRingMax))) | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This channel rejection logic duplicates the rejection already performed in getChannel (which sets ampl to 0). This creates maintainability concerns:
- The rejection conditions must be kept in sync between getChannel and this code
- The behavior is now inconsistent with fillCorrelationsFT0AFT0C (around line 810-833), which still processes channels where ampl=0
Consider either: (a) refactoring getChannel to return a boolean indicating whether to skip the channel, or (b) moving all rejection logic out of getChannel into the calling code and applying it consistently in both fillCorrelationsTPCFT0 and fillCorrelationsFT0AFT0C.
| if (corType == kFT0C) { | |
| if ((cfgRejectFT0CInside && (chanelid >= kFT0CInnerRingMin && chanelid <= kFT0CInnerRingMax)) || (cfgRejectFT0COutside && (chanelid >= kFT0COuterRingMin && chanelid <= kFT0COuterRingMax))) | |
| continue; | |
| } else if (corType == kFT0A) { | |
| if ((cfgRejectFT0AInside && (chanelid >= kFT0AInnerRingMin && chanelid <= kFT0AInnerRingMax)) || (cfgRejectFT0AOutside && (chanelid >= kFT0AOuterRingMin && chanelid <= kFT0AOuterRingMax))) | |
| continue; | |
| } |
|
Please, don't keep closing PRs |
|
okay great, I didn't know that, thank you! |
2 participants
Preserve original amplitude values when filling dead channel histograms
Properly declare mirroredAmpl variables