[PWGCF] Add option to remap dead FT0 channels #14427
Conversation
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 PR adds optional functionality to remap dead FT0 detector channels to their functioning mirrored counterparts, allowing amplitude data from working channels to be used in place of dead ones. The feature is controlled by two new configuration flags (disabled by default) for FT0A and FT0C detectors.
Key changes:
- Added configuration options to enable dead channel remapping for FT0A and FT0C detectors
- Implemented channel remapping logic that copies amplitude data from functioning channels to their mirrored dead channels
- Added enum constants to define mirroring offsets and dead channel ranges
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| kFT0AInnerMirror = 16, | ||
| kFT0COuterMirror = 32, | ||
| kFT0CInnerMirror = 24 | ||
| }; |
There was a problem hiding this comment.
The constant kFT0AInnerMirror is defined but never used in the code. Consider removing it if it's not needed, or add a comment explaining its purpose if it's reserved for future use.
| kFT0AInnerMirror = 16, | |
| kFT0COuterMirror = 32, | |
| kFT0CInnerMirror = 24 | |
| }; | |
| kFT0COuterMirror = 32, | |
| kFT0CInnerMirror = 24 | |
| }; |
| if (cfgRemapFT0CDeadChannels) { | ||
| if (id == kFT0CRemapChannelInnerRing) { | ||
| int dead_id = id + kFT0CInnerMirror; | ||
| registry.fill(HIST("FT0Amp"), dead_id, ampl); | ||
| ampl = ampl / cstFT0RelGain[iCh]; | ||
| registry.fill(HIST("FT0AmpCorrect"), dead_id, ampl); | ||
| } else if (id >= kFT0CRemapChannelStart && id <= kFT0CRemapChannelEnd) { | ||
| int dead_id = id + kFT0COuterMirror; | ||
| registry.fill(HIST("FT0Amp"), dead_id, ampl); | ||
| ampl = ampl / cstFT0RelGain[iCh]; | ||
| registry.fill(HIST("FT0AmpCorrect"), dead_id, ampl); | ||
| } | ||
| } |
There was a problem hiding this comment.
The gain correction is being applied using the wrong index. The code uses cstFT0RelGain[iCh], where iCh is the position in the FT0C channel array, but cstFT0RelGain is indexed by the global channel ID (0-207). For dead channels, you should use cstFT0RelGain[dead_id] instead of cstFT0RelGain[iCh] to apply the correct gain correction for the remapped dead channel.
| if (cfgRemapFT0ADeadChannels) { | ||
| if (id >= kFT0ARemapChannelStart && id <= kFT0ARemapChannelEnd) { | ||
| int dead_id = id - kFT0AOuterMirror; | ||
| registry.fill(HIST("FT0Amp"), dead_id, ampl); | ||
| ampl = ampl / cstFT0RelGain[iCh]; | ||
| registry.fill(HIST("FT0AmpCorrect"), dead_id, ampl); | ||
| } | ||
| } |
There was a problem hiding this comment.
The gain correction is being applied using the wrong index. The code uses cstFT0RelGain[iCh], where iCh is the position in the FT0A channel array, but cstFT0RelGain is indexed by the global channel ID (0-207). For dead channels, you should use cstFT0RelGain[dead_id] instead of cstFT0RelGain[iCh] to apply the correct gain correction for the remapped dead channel.
| if (cfgRemapFT0CDeadChannels) { | ||
| if (id == kFT0CRemapChannelInnerRing) { | ||
| int dead_id = id + kFT0CInnerMirror; | ||
| registry.fill(HIST("FT0Amp"), dead_id, ampl); | ||
| ampl = ampl / cstFT0RelGain[iCh]; | ||
| registry.fill(HIST("FT0AmpCorrect"), dead_id, ampl); | ||
| } else if (id >= kFT0CRemapChannelStart && id <= kFT0CRemapChannelEnd) { | ||
| int dead_id = id + kFT0COuterMirror; | ||
| registry.fill(HIST("FT0Amp"), dead_id, ampl); | ||
| ampl = ampl / cstFT0RelGain[iCh]; | ||
| registry.fill(HIST("FT0AmpCorrect"), dead_id, ampl); | ||
| } | ||
| } |
There was a problem hiding this comment.
The interaction between the remapping and rejection features is potentially confusing. When remapping is enabled, dead channels receive amplitude data from functioning channels. However, if rejection is enabled for the ring containing the dead channels, those dead channels will still have non-zero amplitude (from remapping), while the functioning channels in the same ring are set to zero. Consider documenting this behavior or adding logic to also zero out remapped dead channels when their corresponding ring rejection is enabled.
| if (cfgRemapFT0ADeadChannels) { | ||
| if (id >= kFT0ARemapChannelStart && id <= kFT0ARemapChannelEnd) { | ||
| int dead_id = id - kFT0AOuterMirror; | ||
| registry.fill(HIST("FT0Amp"), dead_id, ampl); | ||
| ampl = ampl / cstFT0RelGain[iCh]; | ||
| registry.fill(HIST("FT0AmpCorrect"), dead_id, ampl); | ||
| } | ||
| } |
There was a problem hiding this comment.
The interaction between the remapping and rejection features is potentially confusing. When remapping is enabled, dead channels receive amplitude data from functioning channels. However, if rejection is enabled for the ring containing the dead channels, those dead channels will still have non-zero amplitude (from remapping), while the functioning channels in the same ring are set to zero. Consider documenting this behavior or adding logic to also zero out remapped dead channels when their corresponding ring rejection is enabled.
victor-gonzalez
left a comment
victor-gonzalez
left a comment
There was a problem hiding this comment.
Any reason for keeping Copilot enabled if you are not considering its suggestions?
2 participants
This PR adds functionality to optionally remap dead FT0 detector channels to their mirrored images, filling amplitude data from functioning channels. By default, the remapping is disabled.