[PWGCF] Adding the option to remap dead channels #14417
Conversation
Adding the option to reject the inner of the outer part of the FT0 detectors
Adding option to reject part of the FT0 detectors
Adding the option to remap dead channels to their mirrored image. The standard is off. This fills the amplitude of 9 dead channels by their mirrored image
Adding the option to remap dead channels
Adding the option to remap dead channels
MartijnLaarhoven
requested review from
alibuild,
ariedel-cern,
glromane,
lauraser,
lgraczykCern,
otonvd,
prchakra,
shouqiye,
victor-gonzalez and
zchochul
as code owners
|
O2 linter results: ❌ 0 errors, |
github-actions
bot
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to remap dead FT0 detector channels by filling their amplitudes with mirrored image data from functional channels. The feature is disabled by default and can be enabled via configuration flags.
Key Changes:
- Added two configurable flags to enable dead channel remapping for FT0A and FT0C detectors
- Implemented channel remapping logic that fills 9 dead channels using their mirrored counterparts
- Added enum definitions for mirroring constants and dead channel identifiers
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ampl = ampl / cstFT0RelGain[iCh]; | ||
| registry.fill(HIST("FT0AmpCorrect"), dead_id, ampl); | ||
| } else if (id >= kFT0CRemapChannelStart && id <= kFT0CRemapChannelEnd) { | ||
| int dead_id = id + kFT0COuterMirror; |
There was a problem hiding this comment.
The variable name dead_id suggests this represents a dead channel ID that should receive data, but the implementation fills histogram data for dead_id using amplitude from the live channel id. This naming is confusing - consider renaming to mirror_id or remap_target_id to clarify that this is the target channel receiving the remapped amplitude.
| ampl = ft0.amplitudeA()[iCh]; | ||
| if (cfgRemapFT0ADeadChannels) { | ||
| if (id >= kFT0ARemapChannelStart && id <= kFT0ARemapChannelEnd) { | ||
| int dead_id = id - kFT0AOuterMirror; |
There was a problem hiding this comment.
The variable name dead_id suggests this represents a dead channel ID that should receive data, but the implementation fills histogram data for dead_id using amplitude from the live channel id. This naming is confusing - consider renaming to mirror_id or remap_target_id to clarify that this is the target channel receiving the remapped amplitude.
| } else if (id >= kFT0CRemapChannelStart && id <= kFT0CRemapChannelEnd) { | ||
| int dead_id = id + kFT0COuterMirror; | ||
| registry.fill(HIST("FT0Amp"), dead_id, ampl); | ||
| ampl = ampl / cstFT0RelGain[iCh]; |
There was a problem hiding this comment.
Incorrect array indexing: cstFT0RelGain[iCh] uses the loop index iCh, but cstFT0RelGain is indexed by channel ID (0-207), not by the position in the sparse channelC() array. This should be cstFT0RelGain[dead_id] since dead_id is the actual channel ID.
| O2_DEFINE_CONFIGURABLE(cfgRejectFT0CInside, bool, false, "Rejection of inner ring channels of the FT0C detector") | ||
| O2_DEFINE_CONFIGURABLE(cfgRejectFT0COutside, bool, false, "Rejection of outer ring channels of the FT0C detector") | ||
| O2_DEFINE_CONFIGURABLE(cfgRemapFT0ADeadChannels, bool, false, "If true, remap FT0A channels 60-63 to amplitudes from 92-95 respectively") | ||
| O2_DEFINE_CONFIGURABLE(cfgRemapFT0CDeadChannels, bool, false, "If true, remap FT0C channels 177->145, 176->144, 178->146, 179->147, 139->115") |
There was a problem hiding this comment.
The configuration description should document all remapped channels, not just the outer ring. Currently it only mentions "177->145, 176->144, 178->146, 179->147, 139->115", but it should clarify that 139->115 is the inner ring channel mapping (using kFT0CInnerMirror = 24).
| O2_DEFINE_CONFIGURABLE(cfgRemapFT0CDeadChannels, bool, false, "If true, remap FT0C channels 177->145, 176->144, 178->146, 179->147, 139->115") | |
| O2_DEFINE_CONFIGURABLE(cfgRemapFT0CDeadChannels, bool, false, "If true, remap FT0C outer ring channels 177->145, 176->144, 178->146, 179->147, and inner ring channel 139->115 (using kFT0CInnerMirror = 24)") |
| if (id == kFT0CRemapChannelInnerRing) { | ||
| int dead_id = id + kFT0CInnerMirror; | ||
| registry.fill(HIST("FT0Amp"), dead_id, ampl); | ||
| ampl = ampl / cstFT0RelGain[iCh]; |
There was a problem hiding this comment.
Incorrect array indexing: cstFT0RelGain[iCh] uses the loop index iCh, but cstFT0RelGain is indexed by channel ID (0-207), not by the position in the sparse channelC() array. This should be cstFT0RelGain[dead_id] since dead_id is the actual channel ID. The same issue affects lines 680 and 696.
| if (id >= kFT0ARemapChannelStart && id <= kFT0ARemapChannelEnd) { | ||
| int dead_id = id - kFT0AOuterMirror; | ||
| registry.fill(HIST("FT0Amp"), dead_id, ampl); | ||
| ampl = ampl / cstFT0RelGain[iCh]; |
There was a problem hiding this comment.
Incorrect array indexing: cstFT0RelGain[iCh] uses the loop index iCh, but cstFT0RelGain is indexed by channel ID (0-207), not by the position in the sparse channelA() array. This should be cstFT0RelGain[dead_id] since dead_id is the actual channel ID.
| O2_DEFINE_CONFIGURABLE(cfgRejectFT0AOutside, bool, false, "Rejection of outer ring channels of the FT0A detector") | ||
| O2_DEFINE_CONFIGURABLE(cfgRejectFT0CInside, bool, false, "Rejection of inner ring channels of the FT0C detector") | ||
| O2_DEFINE_CONFIGURABLE(cfgRejectFT0COutside, bool, false, "Rejection of outer ring channels of the FT0C detector") | ||
| O2_DEFINE_CONFIGURABLE(cfgRemapFT0ADeadChannels, bool, false, "If true, remap FT0A channels 60-63 to amplitudes from 92-95 respectively") |
There was a problem hiding this comment.
The configuration description states "remap FT0A channels 60-63 to amplitudes from 92-95", but the implementation does the opposite: it remaps channels 92-95 to 60-63 (using dead_id = id - 32). Please verify that the implementation matches the intended behavior and update either the code or the description accordingly.
| O2_DEFINE_CONFIGURABLE(cfgRemapFT0ADeadChannels, bool, false, "If true, remap FT0A channels 60-63 to amplitudes from 92-95 respectively") | |
| O2_DEFINE_CONFIGURABLE(cfgRemapFT0ADeadChannels, bool, false, "If true, remap FT0A channels 92-95 to amplitudes from 60-63 respectively") |
| ampl = ft0.amplitudeC()[iCh]; | ||
| if (cfgRemapFT0CDeadChannels) { | ||
| if (id == kFT0CRemapChannelInnerRing) { | ||
| int dead_id = id + kFT0CInnerMirror; |
There was a problem hiding this comment.
The variable name dead_id suggests this represents a dead channel ID that should receive data, but the implementation fills histogram data for dead_id using amplitude from the live channel id. This naming is confusing - consider renaming to mirror_id or remap_target_id to clarify that this is the target channel receiving the remapped amplitude.
|
Please, rebase your changes to the latest version of |
2 participants
Adding the option to remap dead channels to their mirrored image. The standard is off. This fills the amplitude of 9 dead channels by their mirrored image