[PWGCF] Updating the channel mirroring logic and adding an option to fill histograms run by run#14940
Conversation
Updating the channel mirroring logic and adding an option to fill histograms run by run
[PWGCF] Updating the channel mirroring logic
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
There was a problem hiding this comment.
Pull request overview
This PR updates the FT0 channel “mirroring” behavior in the long-range dihadron correlation task and adds an optional feature to produce FT0 amplitude QA histograms on a run-by-run basis.
Changes:
- Reworks FT0 mirroring from channel remapping to additional fills using a mirrored-φ transformation, controlled via new forward-detector configurables.
- Adds optional per-run output histogram creation and bookkeeping to fill FT0 amplitude QA per run.
- Refactors forward-detector configurables into a
ConfigurableGroupand updates PID n-sigma labeling semantics.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #include <memory> | ||
| #include <string> | ||
| #include <utility> | ||
| #include <vector> |
There was a problem hiding this comment.
New code uses std::find (run-by-run run number tracking) but this file does not include <algorithm>. Relying on indirect includes is fragile and can break compilation with different standard library implementations; add #include <algorithm>.
| #include <vector> | |
| #include <vector> | |
| #include <algorithm> |
| 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 labels were changed to upCut_*/lowCut_*, but the help text still says "(positive and negative)" which no longer matches the meaning of the array. Please update the description (and/or labels) so the configuration UI reflects the actual semantics (upper/lower cuts).
| 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 upper (upCut_*) and lower (lowCut_*) cuts for TPC, TOF, ITS for pions, kaons, protons"}; |
| if (mirrorChannelA) { | ||
| sameFt0aFt0c->getPairHist()->Fill(step, fSampleIndex, posZ, 0.5, 0.5, RecoDecay::constrainAngle(phiA + 2 * PIHalf - phiC, -PIHalf), deltaEta, amplA * amplC * eventWeight * triggerWeight); | ||
| if (mirrorChannelC) | ||
| sameFt0aFt0c->getPairHist()->Fill(step, fSampleIndex, posZ, 0.5, 0.5, deltaPhi, deltaEta, amplA * amplC * eventWeight * triggerWeight); | ||
| } |
There was a problem hiding this comment.
When both mirrorChannelA and mirrorChannelC are true, this block fills the pair histogram with deltaPhi a second time (in addition to the unconditional fill above), which will double-count those pairs. If you intended to add the “A mirrored + C mirrored” combination, compute and fill the deltaPhi for that combined mirrored case instead of reusing deltaPhi.
| auto phi = getPhiFT0(chanelid, corType); | ||
| auto eta = getEtaFT0(chanelid, corType); | ||
| if (cfgDrawEtaPhiDis && system == SameEvent) { | ||
| registry.fill(HIST("EtaPhi"), eta, phi, ampl * eventWeight); | ||
| if (mirrorChannel) | ||
| registry.fill(HIST("EtaPhi"), eta, 4 * PIHalf - phi, ampl * eventWeight); | ||
| } | ||
| float deltaPhi = RecoDecay::constrainAngle(track1.phi() - phi, -PIHalf); | ||
| float deltaEta = track1.eta() - eta; | ||
| // fill the right sparse and histograms | ||
| if (system == SameEvent) { | ||
| if (corType == kFT0A) { | ||
| registry.fill(HIST("Assoc_amp_same_TPC_FT0A"), chanelid, ampl); | ||
| sameTpcFt0a->getPairHist()->Fill(step, fSampleIndex, posZ, track1.pt(), track1.pt(), deltaPhi, deltaEta, ampl * eventWeight * triggerWeight); | ||
| registry.fill(HIST("deltaEta_deltaPhi_same_TPC_FT0A"), deltaPhi, deltaEta, ampl * eventWeight * triggerWeight); | ||
| sameTpcFt0a->getPairHist()->Fill(step, fSampleIndex, posZ, track1.pt(), track1.pt(), deltaPhi, deltaEta, ampl * eventWeight * triggerWeight); | ||
| if (mirrorChannel) | ||
| sameTpcFt0a->getPairHist()->Fill(step, fSampleIndex, posZ, track1.pt(), track1.pt(), RecoDecay::constrainAngle(track1.phi() - phi - 2 * PIHalf, -PIHalf), deltaEta, ampl * eventWeight * triggerWeight); | ||
| } else if (corType == kFT0C) { | ||
| registry.fill(HIST("Assoc_amp_same_TPC_FT0C"), chanelid, ampl); | ||
| sameTpcFt0c->getPairHist()->Fill(step, fSampleIndex, posZ, track1.pt(), track1.pt(), deltaPhi, deltaEta, ampl * eventWeight * triggerWeight); | ||
| registry.fill(HIST("deltaEta_deltaPhi_same_TPC_FT0C"), deltaPhi, deltaEta, ampl * eventWeight * triggerWeight); | ||
| sameTpcFt0c->getPairHist()->Fill(step, fSampleIndex, posZ, track1.pt(), track1.pt(), deltaPhi, deltaEta, ampl * eventWeight * triggerWeight); | ||
| if (mirrorChannel) | ||
| sameTpcFt0c->getPairHist()->Fill(step, fSampleIndex, posZ, track1.pt(), track1.pt(), RecoDecay::constrainAngle(track1.phi() - phi - 2 * PIHalf, -PIHalf), deltaEta, ampl * eventWeight * triggerWeight); |
There was a problem hiding this comment.
Mirroring uses different transformations in different places: the eta-phi QA uses 2π - φ (4 * PIHalf - phi), but the correlation fill uses a ±π shift (phi - 2 * PIHalf, phiA ± 2 * PIHalf - phiC). This inconsistency will distort the mirrored correlations relative to the QA visualization. Consider defining a single helper to compute the mirrored φ (e.g. phiMirror = TwoPI - phi) and use it consistently for both QA and deltaPhi calculations.
| 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 in the FT0A branch: histAmpCorrectPerRun[lastRunNumber]->Fill(...) can dereference a null shared_ptr (and operator[] mutates the map) when run-by-run output is disabled or not initialized for the current run. Add a config guard and access the map without operator[].
| if (cfgFwdConfig.cfgRunbyRunAmplitudeFT0) { | ||
| if (histAmpCorrectPerRun.find(runNumber) != histAmpCorrectPerRun.end()) { | ||
| LOGF(info, "you are trying to create QA hist again, please make sure you are not filling it twice"); | ||
| } | ||
| 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}}); | ||
| histAmpCorrectPerRun.insert(std::make_pair(runNumber, histFT0AmpCorrect)); | ||
| } |
There was a problem hiding this comment.
createOutputObjectsForRun() logs when the run histogram already exists but still proceeds to call registry.add and then insert. This can create duplicate registry entries and/or leave the map unchanged (since insert won’t overwrite), making behavior inconsistent. Return early when the key exists, or use try_emplace and only create/add the histogram when insertion succeeds.
| 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.
The comment says this is a map of TH3 histograms, but the type is std::map<int, std::shared_ptr<TH2>>. Please update the comment to match the actual type (TH2) 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 |
| if (system == SameEvent) { | ||
| registry.fill(HIST("FT0AmpCorrect"), id, ampl); | ||
| histAmpCorrectPerRun[lastRunNumber]->Fill(id, ampl); | ||
| } |
There was a problem hiding this comment.
getChannel() unconditionally dereferences histAmpCorrectPerRun[lastRunNumber] for same-event filling. When cfgFwdConfig.cfgRunbyRunAmplitudeFT0 is false (default) or when no histogram was created for lastRunNumber, operator[] will insert a null shared_ptr and this will segfault on ->Fill(). Guard this fill behind cfgFwdConfig.cfgRunbyRunAmplitudeFT0 and use find()/at() (or keep a raw pointer to the current run histogram) to avoid accidental insertion and null dereference.
| } | ||
| 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.
Histogram title string passed to registry.add<TH2> only contains one ';' ("FIT channel;FIT amplitude"), so ROOT will not set the Y-axis label (and the title/X label may not be what you intend). Use the standard "title;xLabel;yLabel" format (e.g. ";FIT channel;FIT amplitude") or a descriptive title with two semicolons.
| std::shared_ptr<TH2> histFT0AmpCorrect = registry.add<TH2>(Form("%d/FT0AmpCorrect", runNumber), "FIT channel;FIT amplitude", {HistType::kTH2F, {axisChID, axisFit}}); | |
| std::shared_ptr<TH2> histFT0AmpCorrect = registry.add<TH2>(Form("%d/FT0AmpCorrect", runNumber), ";FIT channel;FIT amplitude", {HistType::kTH2F, {axisChID, axisFit}}); |
[PWGCF] Updating the channel mirroring logic and adding an option to fill histograms run by run
victor-gonzalez
left a comment
victor-gonzalez
left a comment
There was a problem hiding this comment.
Please, resolve the conflicts
victor-gonzalez
left a comment
victor-gonzalez
left a comment
There was a problem hiding this comment.
The changes should not be merged, they should be rebased to the latest version of the master
3 participants
Updating the channel mirroring logic and adding an option to fill histograms run by run