-
Notifications
You must be signed in to change notification settings - Fork 613
[PWGCF] Adding an option to reject the inner or outher rings of the FT0 detectors, and adding the option to fill the 'dead' parts of the detector with the mirrored parts. #14268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Added option to reject the inside or the outside of the detector ring of FT0
Added option to reject the inside or the outside of the detector ring of FT0
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
making the rejection of the rejection of part of the detector more distinctive
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
O2 linter results: ❌ 9 errors, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds functionality to handle dead channels and selectively reject detector rings in the FT0A and FT0C detectors for long-range dihadron correlation analysis. The implementation allows remapping dead detector channels to mirror working channels and provides options to exclude inner or outer rings from the analysis.
Key changes:
- Added six new configuration options to control FT0 detector channel rejection and remapping
- Introduced a
DetectorChannelsenum defining inner/outer ring boundaries for FT0A and FT0C - Implemented dead channel remapping and ring rejection logic in the
getChannelfunction
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ampl = ft0.amplitudeA()[92]; | ||
| else if (id == 61) | ||
| ampl = ft0.amplitudeA()[93]; | ||
| else if (id == 62) | ||
| ampl = ft0.amplitudeA()[94]; | ||
| else if (id == 63) | ||
| ampl = ft0.amplitudeA()[95]; |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The channel remapping does not match the PR description. The description states "remap FT0A channels 60-63 to amplitudes from 28,30,29,31 respectively", but the code maps channels 60-63 to amplitudes from indices 92,93,94,95. Please verify which mapping is correct and update either the code or the description accordingly.
| ampl = ft0.amplitudeA()[92]; | |
| else if (id == 61) | |
| ampl = ft0.amplitudeA()[93]; | |
| else if (id == 62) | |
| ampl = ft0.amplitudeA()[94]; | |
| else if (id == 63) | |
| ampl = ft0.amplitudeA()[95]; | |
| ampl = ft0.amplitudeA()[28]; | |
| else if (id == 61) | |
| ampl = ft0.amplitudeA()[30]; | |
| else if (id == 62) | |
| ampl = ft0.amplitudeA()[29]; | |
| else if (id == 63) | |
| ampl = ft0.amplitudeA()[31]; |
| id = ft0.channelC()[iCh]; | ||
| id = id + Ft0IndexA; | ||
| ampl = ft0.amplitudeC()[iCh]; | ||
| if (cfgRemapFT0CDeadCha if (id == 177) | ||
| ampl = ft0.amplitudeC()[145]; | ||
| else if (id == 176) | ||
| ampl = ft0.amplitudeC()[144]; | ||
| else if (id == 178) | ||
| ampl = ft0.amplitudeC()[146]; | ||
| else if (id == 179) | ||
| ampl = ft0.amplitudeC()[147]; | ||
| else if (id == 139) | ||
| ampl = ft0.amplitudeC()[115]; | ||
| } | ||
| if ((cfgRejectFT0CInside && (id >= kFT0CInnerRingMin && id <= kFT0CInnerRingMax)) || (cfgRejectFT0COutside && (id >= kFT0COuterRingMin && id <= kFT0COuterRingMax))) | ||
| ampl = 0.; | ||
| registry.fill(HIST("FT0Amp"), id, ampl); | ||
| ampl = ampl / cstFT0RelGain[iCh]; | ||
| registry.fill(HIST("FT0AmpCorrect"), id, ampl); | ||
| } else if (fitType == kFT0A) { | ||
| id = ft0.channelA()[iCh]; | ||
| ampl = ft0.amplitudeA()[iCh]; | ||
| if (cfgRemapFT0ADeadChannels) { | ||
| if (id == 60) | ||
| ampl = ft0.amplitudeA()[92]; | ||
| else if (id == 61) | ||
| ampl = ft0.amplitudeA()[93]; | ||
| else if (id == 62) | ||
| ampl = ft0.amplitudeA()[94]; | ||
| else if (id == 63) | ||
| ampl = ft0.amplitudeA()[95]; | ||
| } | ||
| if ((cfgRejectFT0AInside && (id >= kFT0AInnerRingMin && id <= kFT0AInnerRingMax)) || (cfgRejectFT0AOutside && (id >= kFT0AOuterRingMin && id <= kFT0AOuterRingMax))) | ||
| ampl = 0.; |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The remapping logic incorrectly uses adjusted channel IDs as array indices. The arrays channelC() and amplitudeC() are indexed by position (0 to size-1), not by channel ID. To remap channel 177 to use the amplitude from channel 145, you need to: (1) find the position in the array where channelC()[j] + 96 == 145 (i.e., where channelC()[j] == 49), then (2) use amplitudeC()[j]. The current implementation directly accesses amplitudeC()[145] which attempts to access array index 145, likely causing out-of-bounds access if the array has fewer than 146 elements. The same issue affects all remapping statements in this block.
| id = ft0.channelC()[iCh]; | |
| id = id + Ft0IndexA; | |
| ampl = ft0.amplitudeC()[iCh]; | |
| if (cfgRemapFT0CDeadCha if (id == 177) | |
| ampl = ft0.amplitudeC()[145]; | |
| else if (id == 176) | |
| ampl = ft0.amplitudeC()[144]; | |
| else if (id == 178) | |
| ampl = ft0.amplitudeC()[146]; | |
| else if (id == 179) | |
| ampl = ft0.amplitudeC()[147]; | |
| else if (id == 139) | |
| ampl = ft0.amplitudeC()[115]; | |
| } | |
| if ((cfgRejectFT0CInside && (id >= kFT0CInnerRingMin && id <= kFT0CInnerRingMax)) || (cfgRejectFT0COutside && (id >= kFT0COuterRingMin && id <= kFT0COuterRingMax))) | |
| ampl = 0.; | |
| registry.fill(HIST("FT0Amp"), id, ampl); | |
| ampl = ampl / cstFT0RelGain[iCh]; | |
| registry.fill(HIST("FT0AmpCorrect"), id, ampl); | |
| } else if (fitType == kFT0A) { | |
| id = ft0.channelA()[iCh]; | |
| ampl = ft0.amplitudeA()[iCh]; | |
| if (cfgRemapFT0ADeadChannels) { | |
| if (id == 60) | |
| ampl = ft0.amplitudeA()[92]; | |
| else if (id == 61) | |
| ampl = ft0.amplitudeA()[93]; | |
| else if (id == 62) | |
| ampl = ft0.amplitudeA()[94]; | |
| else if (id == 63) | |
| ampl = ft0.amplitudeA()[95]; | |
| } | |
| if ((cfgRejectFT0AInside && (id >= kFT0AInnerRingMin && id <= kFT0AInnerRingMax)) || (cfgRejectFT0AOutside && (id >= kFT0AOuterRingMin && id <= kFT0AOuterRingMax))) | |
| ampl = 0.; | |
| const auto& channelsC = ft0.channelC(); | |
| const auto& amplitudesC = ft0.amplitudeC(); | |
| id = channelsC[iCh]; | |
| id = id + Ft0IndexA; | |
| ampl = amplitudesC[iCh]; | |
| if (cfgRemapFT0CDeadCha) { | |
| auto getRemappedAmplitudeC = [&](int sourceChannelId) -> float { | |
| for (std::size_t j = 0; j < channelsC.size(); ++j) { | |
| if (channelsC[j] + Ft0IndexA == sourceChannelId) { | |
| return amplitudesC[j]; | |
| } | |
| } | |
| // If the requested source channel is not present, treat it as having zero amplitude. | |
| return 0.f; | |
| }; | |
| if (id == 177) { | |
| ampl = getRemappedAmplitudeC(145); | |
| } else if (id == 176) { | |
| ampl = getRemappedAmplitudeC(144); | |
| } else if (id == 178) { | |
| ampl = getRemappedAmplitudeC(146); | |
| } else if (id == 179) { | |
| ampl = getRemappedAmplitudeC(147); | |
| } else if (id == 139) { | |
| ampl = getRemappedAmplitudeC(115); | |
| } | |
| } | |
| if ((cfgRejectFT0CInside && (id >= kFT0CInnerRingMin && id <= kFT0CInnerRingMax)) || (cfgRejectFT0COutside && (id >= kFT0COuterRingMin && id <= kFT0COuterRingMax))) | |
| ampl = 0.f; | |
| registry.fill(HIST("FT0Amp"), id, ampl); | |
| ampl = ampl / cstFT0RelGain[iCh]; | |
| registry.fill(HIST("FT0AmpCorrect"), id, ampl); | |
| } else if (fitType == kFT0A) { | |
| const auto& channelsA = ft0.channelA(); | |
| const auto& amplitudesA = ft0.amplitudeA(); | |
| id = channelsA[iCh]; | |
| ampl = amplitudesA[iCh]; | |
| if (cfgRemapFT0ADeadChannels) { | |
| auto getRemappedAmplitudeA = [&](int sourceChannelId) -> float { | |
| for (std::size_t j = 0; j < channelsA.size(); ++j) { | |
| if (channelsA[j] == sourceChannelId) { | |
| return amplitudesA[j]; | |
| } | |
| } | |
| // If the requested source channel is not present, treat it as having zero amplitude. | |
| return 0.f; | |
| }; | |
| if (id == 60) { | |
| ampl = getRemappedAmplitudeA(92); | |
| } else if (id == 61) { | |
| ampl = getRemappedAmplitudeA(93); | |
| } else if (id == 62) { | |
| ampl = getRemappedAmplitudeA(94); | |
| } else if (id == 63) { | |
| ampl = getRemappedAmplitudeA(95); | |
| } | |
| } | |
| if ((cfgRejectFT0AInside && (id >= kFT0AInnerRingMin && id <= kFT0AInnerRingMax)) || (cfgRejectFT0AOutside && (id >= kFT0AOuterRingMin && id <= kFT0AOuterRingMax))) | |
| ampl = 0.f; |
| id = ft0.channelC()[iCh]; | ||
| id = id + Ft0IndexA; | ||
| ampl = ft0.amplitudeC()[iCh]; | ||
| if (cfgRemapFT0CDeadCha if (id == 177) |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Syntax error: The line appears to be corrupted or incomplete. The condition if (cfgRemapFT0CDeadCha is truncated and missing the closing parenthesis and opening brace. It should likely be if (cfgRemapFT0CDeadChannels) { to match the pattern used for FT0A on line 677.
| if (cfgRemapFT0CDeadCha if (id == 177) | |
| if (cfgRemapFT0CDeadChannels) { | |
| if (id == 177) |
|
Please always validate your formatting before opening a PR for review to avoid useless compilation tests. |
Your code doesn't even compile! |
Adding an option to reject the inner or outher rings of the FT0 detectors, and adding the option to fill the 'dead' parts of the detector with the mirrored parts.