does not try to download sampmax calibs etc for sims #4149
Conversation
📝 WalkthroughWalkthroughAdds a text-file writer for per-channel time-correction data (new MbdCalib::Write_TimeCorr) and refactors InitRun/channel initialization control flow in MbdEvent (conditional Download_All for real data and reordered pedestal/template setup). No new public types beyond the Write_TimeCorr declaration. Changes
✨ Finishing touches
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 |
mchiu-bnl
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
offline/packages/mbd/MbdCalib.h (1)
149-156:⚠️ Potential issue | 🟠 MajorDocument downstream impact of the new public
Write_TimeCorrAPI.Line 155 adds a new public method; please include compatibility notes and any downstream update requirements (e.g., macros/tools that will call it) so consumers can adjust.
As per coding guidelines: If interfaces change, ask for compatibility notes and any needed downstream updates.
offline/packages/mbd/MbdEvent.cc (2)
155-214:⚠️ Potential issue | 🟠 MajorSimulation runs now skip per‑channel signal initialization.
Line 186–214 moved
SetCalib, pedestal config, and template setup under_simflag==0. For simulations, those calls are skipped, but later processing still uses_mbdsigand_mbdcal(e.g., sampmax/tcorr), risking null/unstable defaults and altered sim output or crashes. Consider keeping the per‑channel initialization outside the real‑data download guard, with a safe pedestal path for sims.✅ Suggested fix (keep download data‑only, but always init channels)
- if ( _simflag == 0 ) // do following for real data - { - // Download calibrations - int status = _mbdcal->Download_All(); - if ( status == -1 ) - { - return Fun4AllReturnCodes::ABORTRUN; - } + if ( _simflag == 0 ) // do following for real data + { + // Download calibrations + int status = _mbdcal->Download_All(); + if ( status == -1 ) + { + return Fun4AllReturnCodes::ABORTRUN; + } ... - for (int ifeech = 0; ifeech < MbdDefs::BBC_N_FEECH; ifeech++) - { - _mbdsig[ifeech].SetCalib(_mbdcal); - if ( _calpass==1 || _is_online || _no_sampmax>0 ) - { - _mbdsig[ifeech].SetEventPed0Range(0,1); - } - else - { - const int presamp = 5; - const int nsamps = -1; - _mbdsig[ifeech].SetEventPed0PreSamp(presamp, nsamps, _mbdcal->get_sampmax(ifeech)); - } - if ( do_templatefit && _mbdgeom->get_type(ifeech)==1 ) - { - _mbdsig[ifeech].SetTemplate(_mbdcal->get_shape(ifeech), _mbdcal->get_sherr(ifeech)); - _mbdsig[ifeech].SetMinMaxFitTime(_mbdcal->get_sampmax(ifeech) - 2 - 3, _mbdcal->get_sampmax(ifeech) - 2 + 3); - } - } - } + } + + for (int ifeech = 0; ifeech < MbdDefs::BBC_N_FEECH; ifeech++) + { + _mbdsig[ifeech].SetCalib(_mbdcal); + const bool use_simple_ped = (_simflag != 0) || _calpass==1 || _is_online || _no_sampmax>0; + if ( use_simple_ped ) + { + _mbdsig[ifeech].SetEventPed0Range(0,1); + } + else + { + const int presamp = 5; + const int nsamps = -1; + _mbdsig[ifeech].SetEventPed0PreSamp(presamp, nsamps, _mbdcal->get_sampmax(ifeech)); + } + if ( do_templatefit && _mbdgeom->get_type(ifeech)==1 ) + { + _mbdsig[ifeech].SetTemplate(_mbdcal->get_shape(ifeech), _mbdcal->get_sherr(ifeech)); + _mbdsig[ifeech].SetMinMaxFitTime(_mbdcal->get_sampmax(ifeech) - 2 - 3, _mbdcal->get_sampmax(ifeech) - 2 + 3); + } + }
155-214:⚠️ Potential issue | 🟠 MajorPlease document analysis impact / reprocessing for sim‑behavior change.
This change alters simulation behavior by skipping calibration downloads and initialization flow. Please state the expected analysis impact and whether any reprocessing is required.
Based on learnings: If the PR changes reconstruction outputs, calibration constants, or simulation behavior, ensure the description states expected analysis impact and whether reprocessing is required.
Build & test reportReport for commit fde222e972808184602e8af9ca51cef370d1286c:
Automatically generated by sPHENIX Jenkins continuous integration |
|
clang-tidy errors are from changed includes in other packages |
pinkenburg
merged commit 30fc305
into
sPHENIX-Collaboration:master
2 participants
comment: Changed so that calibrations are only downloaded for real data, not sims. Previously this worked because if it didn't find the calibs, the code would just ignore that the calibs didn't exist, or used default values which did nothing.
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 Download for Simulations - Conditional Check
Motivation / Context
The MBD calibration loading code attempted to download and validate calibration files even for simulation runs, causing unnecessary I/O and reliance on calibrations that are not applicable to sims. The change gates calibration downloads and related initialization to real-data runs and adds a text output helper for time-correction data.
Key changes
Potential risk areas
Possible future improvements
Note: AI-assisted summary — please review the diffs directly; automated summarization can make mistakes or miss subtle contextual details.