mods for mbd calib passes to work with waveform fit dsts#4262
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new MbdCalibReco SubsysReco (declarations, lifecycle, histogram management, output file) and integrates it into the offline build; updates MbdEvent to conditionally load local calibration files via std::filesystem and to restrict on-the-fly sampmax completion to pass0. ChangesMBD Calibration Subsystem (MbdCalibReco)
MbdEvent Calibration File Loading Integration
Sequence Diagram(s)sequenceDiagram
participant Framework
participant MbdCalibReco
participant MbdCalib
participant TFile
Framework->>MbdCalibReco: Init()
MbdCalibReco->>MbdCalib: construct & configure
Framework->>MbdCalibReco: InitRun(topNode)
MbdCalibReco->>MbdCalib: load baseline calibs (CDB or local)
MbdCalibReco->>TFile: open per-subpass output
Framework->>MbdCalibReco: process_event()
MbdCalibReco->>MbdCalib: read hits, fill histograms
Framework->>MbdCalibReco: EndRun(runnumber)
MbdCalibReco->>TFile: write histograms & close
Possibly related PRs
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
offline/packages/mbd/MbdEvent.cc (1)
168-199:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMajor: silent miscalibration risk when local pass1 calibs are absent at calpass ≥ 2.
For
_calpass > 1you only log "skipping" whenmbd_sampmax.calib/mbd_ped.calibare missing (lines 174–177, 185–188). The on‑the‑fly sampmax fallback at line 194 is now gated on_calpass == 0, so at calpass ≥ 2 a missing sampmax (from both local file and CDB) leaves_mbdcalwith default values, and_mbdsig[ifeech].SetEventPed0PreSamp(... _mbdcal->get_sampmax(ifeech))is then called with that bogus value — silently producing wrong charge/time calibrations rather than aborting. Please eitherreturn Fun4AllReturnCodes::ABORTRUN(or equivalent) whenscheck < 0at_calpass > 1, or assert thatDownload_All()populated sampmax before reaching the pre-sample setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3195982-c147-43e1-bfcc-735f8a2dbe88
📒 Files selected for processing (4)
offline/packages/mbd/Makefile.amoffline/packages/mbd/MbdCalibReco.ccoffline/packages/mbd/MbdCalibReco.hoffline/packages/mbd/MbdEvent.cc
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
offline/packages/mbd/MbdCalibReco.cc (2)
56-57:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPropagate
getNodesfailure inInitRunto prevent null dereferences laterAt Line 56,
getNodes(topNode)is called but its status is ignored. If any required node is missing,getNodesreturnsABORTRUN, yet initialization continues;process_eventthen dereferences pointers like_gl1packet(Line 312) and_mbdout(Line 324).Proposed fix
- getNodes(topNode); + const int node_status = getNodes(topNode); + if (node_status != Fun4AllReturnCodes::EVENT_OK) + { + return node_status; + }As per coding guidelines, for
**/*.{cc,cpp,cxx,c}prioritize correctness, memory safety, error handling, and thread-safety.
237-242:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winHistogram rebooking currently leaves dangling pointers and skips re-allocation
At Lines 237-242,
BookHistograms()deletes existing histograms and immediately returns, so nothing is rebooked for the new run. Combined with Lines 281-305 (pointers not nulled afterdelete), later accesses inprocess_event/EndRuncan hit freed objects or double-delete paths.Proposed fix
void MbdCalibReco::BookHistograms() { // Delete histograms if they have already have been booked. if ( h2_tt ) { DeleteHistograms(); - return; } ... } void MbdCalibReco::DeleteHistograms() { for (int ipmt = 0; ipmt < MbdDefs::MBD_N_PMT; ipmt++) { - if ( h_tt[ipmt] ) - { - delete h_tt[ipmt]; - } - if ( h_tq[ipmt] ) - { - delete h_tq[ipmt]; - } - if ( h_qp[ipmt] ) - { - delete h_qp[ipmt]; - } - if ( h2_slew[ipmt] ) - { - delete h2_slew[ipmt]; - } + if (h_tt[ipmt]) { delete h_tt[ipmt]; h_tt[ipmt] = nullptr; } + if (h_tq[ipmt]) { delete h_tq[ipmt]; h_tq[ipmt] = nullptr; } + if (h_qp[ipmt]) { delete h_qp[ipmt]; h_qp[ipmt] = nullptr; } + if (h2_slew[ipmt]) { delete h2_slew[ipmt]; h2_slew[ipmt] = nullptr; } } - delete h2_tt; - delete h2_tq; + delete h2_tt; h2_tt = nullptr; + delete h2_tq; h2_tq = nullptr; }As per coding guidelines, for
**/*.{cc,cpp,cxx,c}prioritize correctness, memory safety, error handling, and thread-safety, and flag unclear lifetime assumptions.Also applies to: 281-305
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 02d2fe25-88d9-412c-babe-8280357ae61e
📒 Files selected for processing (3)
offline/packages/mbd/MbdCalibReco.ccoffline/packages/mbd/MbdCalibReco.hoffline/packages/mbd/MbdEvent.cc
🚧 Files skipped from review as they are similar to previous changes (2)
- offline/packages/mbd/MbdCalibReco.h
- offline/packages/mbd/MbdEvent.cc
Build & test reportReport for commit 157bed5a43cd6ef8ce18239de8f4a06c78b147c9:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 3a6becdf82fb2b9aa2bdcacf780d8479081c082b:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 94ccd3502c6acd8184d8bdd5b7f095becd5e5867:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 5bbb0c2aeab8a1aeffc98ef9699109adcbccc1bd:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 4134ac0bf46aaf1f7c7ce15fb4a455c3e865d121:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit c7e5387d425f592730b11c385a61624a9ad46d2d:
Automatically generated by sPHENIX Jenkins continuous integration |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
comment: Changes needed for the MBD calibrations to work with the waveform fit DSTs.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
MBD Calibration Module Updates for Waveform-Fit DSTs
Motivation
This PR extends the MBD (Muon Background Detector) calibration pipeline to function with waveform-fit DSTs by introducing a new histogram-based calibration processor (
MbdCalibReco) and refining the calibration pass logic in the main event processor to support multi-pass calibration workflows with local calibration file fallbacks.Key Changes
New
MbdCalibRecosubsystem processor (header + implementation):SubsysRecoand manages per-event histogram booking for calibration analysisSetSubPass(),SetCalDir(), andSetCDBTag().calibfilesMbdEvent.cccalibration logic refinement:<filesystem>checks for local calibration files before attempting CDB/hardcoded loads_calpasshandling: gates "on-the-fly" sampmax determination to_calpass == 0(was_calpass != 1)_calpass == 2, conditionally loads pass-2-specific calibration files (mbd_tt_t0.calib,mbd_tq_t0.calib,mbd_qfit.calib) instead of unconditionally resetting correctionsBuild system: Exports new
MbdCalibReco.hheader and addsMbdCalibReco.ccto library sourcesPotential Risk Areas
_calpasslogic may alter sampling, pedestal, and timing-correction completeness conditions across the three-pass workflow.calibfiles inresults/<run>/directory; missing files silently degrade to defaults rather than failingMbdCalibRecoalongside existing outputsAI-generated summaries can contain inaccuracies. Review histogram allocation logic, calibration file path construction, and trigger/vertex cut thresholds manually.
Possible Future Improvements