Conversation
There was a problem hiding this comment.
Pull request overview
Updates the LongRangeDihadronCor analysis task to support FT0 dead-channel mirroring and optional run-by-run FT0 amplitude QA output within the existing histogram registry/correlation-filling flow.
Changes:
- Refactors forward-detector (FT0) configurables into a dedicated
cfgFwdConfiggroup and introduces new mirroring/run-by-run QA toggles. - Adds optional per-run
FT0AmpCorrecthistogram creation and filling keyed by run number. - Introduces “mirror channel” handling to duplicate correlation entries for specific FT0 channels intended to compensate dead channels.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ampl = ampl / cstFT0RelGain[iCh]; | ||
| registry.fill(HIST("FT0AmpCorrect"), id, ampl); | ||
| if (system == SameEvent) { | ||
| registry.fill(HIST("FT0AmpCorrect"), id, ampl); | ||
| histAmpCorrectPerRun[lastRunNumber]->Fill(id, ampl); | ||
| } |
There was a problem hiding this comment.
histAmpCorrectPerRun[lastRunNumber]->Fill(...) is executed for every same-event FT0C channel, even when cfgFwdConfig.cfgRunbyRunAmplitudeFT0 is false (and no per-run histograms are created). This can dereference a null shared_ptr and crash; also operator[] will insert a missing key with a null value. Guard this fill behind cfgRunbyRunAmplitudeFT0, and use find/at (or contains) to access the histogram after ensuring it exists.
| ampl = ampl / cstFT0RelGain[iCh]; | ||
| registry.fill(HIST("FT0AmpCorrect"), id, ampl); | ||
| if (system == SameEvent) { | ||
| registry.fill(HIST("FT0AmpCorrect"), id, ampl); | ||
| histAmpCorrectPerRun[lastRunNumber]->Fill(id, ampl); | ||
| } |
There was a problem hiding this comment.
Same issue as above for the FT0A branch: histAmpCorrectPerRun[lastRunNumber]->Fill(...) is not guarded by cfgFwdConfig.cfgRunbyRunAmplitudeFT0 and uses operator[], which can create a null entry and then dereference it. Please gate the per-run fill on the config and access the map via find/at after a successful creation/check.
| } | ||
| const AxisSpec axisFit{1000, 0, 5000, "FIT amplitude"}; | ||
| const AxisSpec axisChID{220, 0, 220, "FIT channel"}; | ||
| std::shared_ptr<TH2> histFT0AmpCorrect = registry.add<TH2>(Form("%d/FT0AmpCorrect", runNumber), "FIT channel;FIT amplitude", {HistType::kTH2F, {axisChID, axisFit}}); |
There was a problem hiding this comment.
createOutputObjectsForRun() logs when the run histogram already exists, but still calls registry.add(...) with the same name and then insert(...) (which won’t overwrite). Depending on HistogramRegistry behavior, this can lead to duplicate-name errors or leaked/unused objects. Return early when the run is already present (or use try_emplace + only registry.add on first insert). Also consider using an empty histogram title string here since axis titles are already provided by AxisSpec (ROOT parses title;x;y).
| } | |
| const AxisSpec axisFit{1000, 0, 5000, "FIT amplitude"}; | |
| const AxisSpec axisChID{220, 0, 220, "FIT channel"}; | |
| std::shared_ptr<TH2> histFT0AmpCorrect = registry.add<TH2>(Form("%d/FT0AmpCorrect", runNumber), "FIT channel;FIT amplitude", {HistType::kTH2F, {axisChID, axisFit}}); | |
| return; | |
| } | |
| const AxisSpec axisFit{1000, 0, 5000, "FIT amplitude"}; | |
| const AxisSpec axisChID{220, 0, 220, "FIT channel"}; | |
| std::shared_ptr<TH2> histFT0AmpCorrect = | |
| registry.add<TH2>(Form("%d/FT0AmpCorrect", runNumber), "", {HistType::kTH2F, {axisChID, axisFit}}); |
| if (mirrorChannel) | ||
| registry.fill(HIST("EtaPhi"), eta, 4 * PIHalf - phi, ampl * eventWeight); |
There was a problem hiding this comment.
The mirrored EtaPhi fill uses 4 * PIHalf - phi (i.e. 2π - φ), but the correlation mirroring uses a π shift (e.g. track1.phi() - phi - π). These are different transformations and will put the mirrored QA entries at inconsistent locations. If the intent is to mirror to the opposite channel, use a φ + π (wrapped to [0, 2π)) consistently.
| if (mirrorChannel) | |
| registry.fill(HIST("EtaPhi"), eta, 4 * PIHalf - phi, ampl * eventWeight); | |
| if (mirrorChannel) { | |
| auto phiMirror = phi + 2 * PIHalf; // mirror to opposite channel: φ + π | |
| if (phiMirror >= 4 * PIHalf) { | |
| phiMirror -= 4 * PIHalf; | |
| } | |
| registry.fill(HIST("EtaPhi"), eta, phiMirror, ampl * eventWeight); | |
| } |
| if (cfgFwdConfig.cfgRunbyRunAmplitudeFT0 && currentRunNumber != lastRunNumber) { | ||
| lastRunNumber = currentRunNumber; | ||
| if (std::find(runNumbers.begin(), runNumbers.end(), currentRunNumber) == runNumbers.end()) { | ||
| // if run number is not in the preconfigured list, create new output histograms for this run | ||
| createOutputObjectsForRun(currentRunNumber); | ||
| runNumbers.push_back(currentRunNumber); |
There was a problem hiding this comment.
This run-by-run block introduces std::find over runNumbers, but the file doesn’t include <algorithm> (and also now relies on <map>/<memory>). This may fail to compile depending on transitive includes. Also, runNumbers is redundant since histAmpCorrectPerRun.find(currentRunNumber) already indicates whether the run was seen—consider using only the map to avoid O(n) scans.
| O2_DEFINE_CONFIGURABLE(cfgPIDParticle, int, 0, "1 = pion, 2 = kaon, 3 = proton, 4 = kshort, 5 = lambda, 6 = phi, 0 for no PID") | ||
| O2_DEFINE_CONFIGURABLE(cfgTofPtCut, float, 0.5f, "Minimum pt to use TOF N-sigma") | ||
| Configurable<LabeledArray<float>> nSigmas{"nSigmas", {LongArrayFloat[0], 3, 6, {"TPC", "TOF", "ITS"}, {"pos_pi", "pos_ka", "pos_pr", "neg_pi", "neg_ka", "neg_pr"}}, "Labeled array for n-sigma values for TPC, TOF, ITS for pions, kaons, protons (positive and negative)"}; | ||
| Configurable<LabeledArray<float>> nSigmas{"nSigmas", {LongArrayFloat[0], 3, 6, {"TPC", "TOF", "ITS"}, {"upCut_pi", "upCut_ka", "upCut_pr", "lowCut_pi", "lowCut_ka", "lowCut_pr"}}, "Labeled array for n-sigma values for TPC, TOF, ITS for pions, kaons, protons (positive and negative)"}; |
There was a problem hiding this comment.
The nSigmas labeled array now uses upCut_*/lowCut_* labels, but the description still says "(positive and negative)". Please update the description to match the new semantics (upper/lower Nσ cuts) to avoid confusing configuration users.
| Configurable<LabeledArray<float>> nSigmas{"nSigmas", {LongArrayFloat[0], 3, 6, {"TPC", "TOF", "ITS"}, {"upCut_pi", "upCut_ka", "upCut_pr", "lowCut_pi", "lowCut_ka", "lowCut_pr"}}, "Labeled array for n-sigma values for TPC, TOF, ITS for pions, kaons, protons (positive and negative)"}; | |
| Configurable<LabeledArray<float>> nSigmas{"nSigmas", {LongArrayFloat[0], 3, 6, {"TPC", "TOF", "ITS"}, {"upCut_pi", "upCut_ka", "upCut_pr", "lowCut_pi", "lowCut_ka", "lowCut_pr"}}, "Labeled array for n-sigma values for TPC, TOF, ITS for pions, kaons, protons (upper and lower N-sigma cuts)"}; |
| std::array<float, 6> tpcNsigmaCut; | ||
| int lastRunNumber = -1; | ||
| std::vector<int> runNumbers; | ||
| std::map<int, std::shared_ptr<TH2>> histAmpCorrectPerRun; // map of TH3 histograms for all runs |
There was a problem hiding this comment.
Typo/inaccuracy in the comment: histAmpCorrectPerRun is declared as std::map<int, std::shared_ptr<TH2>> but the trailing comment says "TH3 histograms". Please fix the comment to avoid confusion.
| std::map<int, std::shared_ptr<TH2>> histAmpCorrectPerRun; // map of TH3 histograms for all runs | |
| std::map<int, std::shared_ptr<TH2>> histAmpCorrectPerRun; // map of TH2 histograms for all runs |
1 participant
No description provided.