added MbdCalib::Write_SlewCorr and MbdCalib::Save_CDB_URL#4273
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR refactors MBD calibration download logic to cache and reuse calibration URLs via a new ChangesMBD Calibration Persistence and URL Management
Possibly related PRs
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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 657d0b19-bf08-4495-b871-d22c922a96b5
📒 Files selected for processing (3)
offline/packages/mbd/MbdCalib.ccoffline/packages/mbd/MbdCalib.hoffline/packages/mbd/MbdEvent.h
Comment on lines
+84
to
+189
| _cdb_urls["MBD_STATUS"] = _cdb->getUrl("MBD_STATUS"); | ||
| if ( !_cdb_urls["MBD_STATUS"].empty() ) | ||
| { | ||
| // if this doesn't exist, the status is assumed to be all good | ||
| Download_Status(status_url); | ||
| Download_Status(_cdb_urls["MBD_STATUS"]); | ||
| } | ||
| if (Verbosity() > 0) | ||
| { | ||
| std::cout << "status_url " << status_url << std::endl; | ||
| std::cout << "MBD_STATUS url " << _cdb_urls["MBD_STATUS"] << std::endl; | ||
| } | ||
|
|
||
| if ( !_rawdstflag ) | ||
| // note: sampmax and ped will be calculated on the fly if calibs don't exist | ||
| _cdb_urls["MBD_SAMPMAX"] = _cdb->getUrl("MBD_SAMPMAX"); | ||
| if (Verbosity() > 0) | ||
| { | ||
| // sampmax and ped will be calculated on the fly if calibs don't exist | ||
| std::string sampmax_url = _cdb->getUrl("MBD_SAMPMAX"); | ||
| if (Verbosity() > 0) | ||
| { | ||
| std::cout << "sampmax_url " << sampmax_url << std::endl; | ||
| } | ||
| Download_SampMax(sampmax_url); | ||
| std::cout << "MBD_SAMPMAX url " << _cdb_urls["MBD_SAMPMAX"] << std::endl; | ||
| } | ||
| Download_SampMax(_cdb_urls["MBD_SAMPMAX"]); | ||
|
|
||
| std::string ped_url = _cdb->getUrl("MBD_PED"); | ||
| if ( !_rawdstflag ) | ||
| { | ||
| _cdb_urls["MBD_PED"] = _cdb->getUrl("MBD_PED"); | ||
| if (Verbosity() > 0) | ||
| { | ||
| std::cout << "ped_url " << ped_url << std::endl; | ||
| std::cout << "MBD_PED url " << _cdb_urls["MBD_PED"] << std::endl; | ||
| } | ||
| Download_Ped(ped_url); | ||
| Download_Ped(_cdb_urls["MBD_PED"]); | ||
|
|
||
| std::string pileup_url = _cdb->getUrl("MBD_PILEUP"); | ||
| if ( pileup_url.empty() ) | ||
| _cdb_urls["MBD_PILEUP"] = _cdb->getUrl("MBD_PILEUP"); | ||
| if ( _cdb_urls["MBD_PILEUP"].empty() ) | ||
| { | ||
| std::cerr << "ERROR, MBD_PILEUP missing" << std::endl; | ||
| return -1; | ||
| } | ||
| if (Verbosity() > 0) | ||
| { | ||
| std::cout << "pileup_url " << pileup_url << std::endl; | ||
| std::cout << "MBD_PILEUP url " << _cdb_urls["MBD_PILEUP"] << std::endl; | ||
| } | ||
| Download_Pileup(pileup_url); | ||
| Download_Pileup(_cdb_urls["MBD_PILEUP"]); | ||
|
|
||
| if (do_templatefit) | ||
| { | ||
| std::string shape_url = _cdb->getUrl("MBD_SHAPES"); | ||
| if ( shape_url.empty() ) | ||
| _cdb_urls["MBD_SHAPES"] = _cdb->getUrl("MBD_SHAPES"); | ||
| if ( _cdb_urls["MBD_SHAPES"].empty() ) | ||
| { | ||
| std::cerr << "ERROR, MBD_SHAPES missing" << std::endl; | ||
| return -1; | ||
| } | ||
| if (Verbosity() > 0) | ||
| { | ||
| std::cout << "shape_url " << shape_url << std::endl; | ||
| std::cout << "MBD_SHAPES url " << _cdb_urls["MBD_SHAPES"] << std::endl; | ||
| } | ||
| Download_Shapes(shape_url); | ||
| Download_Shapes(_cdb_urls["MBD_SHAPES"]); | ||
| } | ||
| } | ||
|
|
||
| if ( !_fitsonly ) | ||
| { | ||
| std::string qfit_url = _cdb->getUrl("MBD_QFIT"); | ||
| _cdb_urls["MBD_QFIT"] = _cdb->getUrl("MBD_QFIT"); | ||
| if (Verbosity() > 0) | ||
| { | ||
| std::cout << "qfit_url " << qfit_url << std::endl; | ||
| std::cout << "MBD_QFIT url " << _cdb_urls["MBD_QFIT"] << std::endl; | ||
| } | ||
| Download_Gains(qfit_url); | ||
| Download_Gains(_cdb_urls["MBD_QFIT"]); | ||
|
|
||
| std::string tt_t0_url = _cdb->getUrl("MBD_TT_T0"); | ||
| _cdb_urls["MBD_TT_T0"] = _cdb->getUrl("MBD_TT_T0"); | ||
| if ( Verbosity() > 0 ) | ||
| { | ||
| std::cout << "tt_t0_url " << tt_t0_url << std::endl; | ||
| std::cout << "MBD_TT_T0 url " << _cdb_urls["MBD_TT_T0"] << std::endl; | ||
| } | ||
| Download_TTT0(tt_t0_url); | ||
| Download_TTT0(_cdb_urls["MBD_TT_T0"]); | ||
|
|
||
| std::string tq_t0_url = _cdb->getUrl("MBD_TQ_T0"); | ||
| _cdb_urls["MBD_TQ_T0"] = _cdb->getUrl("MBD_TQ_T0"); | ||
| if (Verbosity() > 0) | ||
| { | ||
| std::cout << "tq_t0_url " << tq_t0_url << std::endl; | ||
| std::cout << "MBD_TQ_T0 url " << _cdb_urls["MBD_TQ_T0"] << std::endl; | ||
| } | ||
| Download_TQT0(tq_t0_url); | ||
| Download_TQT0(_cdb_urls["MBD_TQ_T0"]); | ||
|
|
||
| std::string t0corr_url = _cdb->getUrl("MBD_T0CORR"); | ||
| _cdb_urls["MBD_T0CORR"] = _cdb->getUrl("MBD_T0CORR"); | ||
| if ( Verbosity() > 0 ) | ||
| { | ||
| std::cout << "t0corr_url " << t0corr_url << std::endl; | ||
| std::cout << "MBD_T0CORR url " << _cdb_urls["MBD_T0CORR"] << std::endl; | ||
| } | ||
| Download_T0Corr(t0corr_url); | ||
| Download_T0Corr(_cdb_urls["MBD_T0CORR"]); | ||
|
|
||
| std::string timecorr_url = _cdb->getUrl("MBD_TIMECORR"); | ||
| _cdb_urls["MBD_TIMECORR"] = _cdb->getUrl("MBD_TIMECORR"); | ||
| if ( Verbosity() > 0 ) | ||
| { | ||
| std::cout << "timecorr_url " << timecorr_url << std::endl; | ||
| std::cout << "MBD_TIMECORR url " << _cdb_urls["MBD_TIMECORR"] << std::endl; | ||
| } | ||
| Download_TimeCorr(timecorr_url); | ||
| Download_TimeCorr(_cdb_urls["MBD_TIMECORR"]); | ||
|
|
||
| std::string slew_url = _cdb->getUrl("MBD_SLEWCORR"); | ||
| _cdb_urls["MBD_SLEWCORR"] = _cdb->getUrl("MBD_SLEWCORR"); | ||
| if ( Verbosity() > 0 ) | ||
| { | ||
| std::cout << "slew_url " << slew_url << std::endl; | ||
| std::cout << "MBD_SLEWCORR url " << _cdb_urls["MBD_SLEWCORR"] << std::endl; | ||
| } | ||
| Download_SlewCorr(slew_url); | ||
| Download_SlewCorr(_cdb_urls["MBD_SLEWCORR"]); | ||
|
|
||
| std::string trms_url = _cdb->getUrl("MBD_TIMERMS"); | ||
| _cdb_urls["MBD_TIMERMS"] = _cdb->getUrl("MBD_TIMERMS"); | ||
| if ( Verbosity() > 0 ) | ||
| { | ||
| std::cout << "trms_url " << trms_url << std::endl; | ||
| std::cout << "MBD_TIMERMS url " << _cdb_urls["MBD_TIMERMS"] << std::endl; | ||
| } | ||
| Download_TimeRMS(trms_url); | ||
| Download_TimeRMS(_cdb_urls["MBD_TIMERMS"]); |
Contributor
There was a problem hiding this comment.
Clear _cdb_urls before rebuilding it.
This map is only refreshed for calibrations enabled by the current _rawdstflag, _fitsonly, and do_templatefit branches. If the same MbdCalib instance is reused, old keys survive and Save_CDB_URL() will persist stale URLs that were not part of the current download.
Proposed fix
if (!_rc->FlagExist("MBD_CALDIR"))
{
+ _cdb_urls.clear();
+
// Always load Status
_cdb_urls["MBD_STATUS"] = _cdb->getUrl("MBD_STATUS");As per coding guidelines, flag hidden global state and unclear lifetime assumptions.
Comment on lines
140
to
+161
| if ( !_fitsonly ) | ||
| { | ||
| std::string qfit_url = _cdb->getUrl("MBD_QFIT"); | ||
| _cdb_urls["MBD_QFIT"] = _cdb->getUrl("MBD_QFIT"); | ||
| if (Verbosity() > 0) | ||
| { | ||
| std::cout << "qfit_url " << qfit_url << std::endl; | ||
| std::cout << "MBD_QFIT url " << _cdb_urls["MBD_QFIT"] << std::endl; | ||
| } | ||
| Download_Gains(qfit_url); | ||
| Download_Gains(_cdb_urls["MBD_QFIT"]); | ||
|
|
||
| std::string tt_t0_url = _cdb->getUrl("MBD_TT_T0"); | ||
| _cdb_urls["MBD_TT_T0"] = _cdb->getUrl("MBD_TT_T0"); | ||
| if ( Verbosity() > 0 ) | ||
| { | ||
| std::cout << "tt_t0_url " << tt_t0_url << std::endl; | ||
| std::cout << "MBD_TT_T0 url " << _cdb_urls["MBD_TT_T0"] << std::endl; | ||
| } | ||
| Download_TTT0(tt_t0_url); | ||
| Download_TTT0(_cdb_urls["MBD_TT_T0"]); | ||
|
|
||
| std::string tq_t0_url = _cdb->getUrl("MBD_TQ_T0"); | ||
| _cdb_urls["MBD_TQ_T0"] = _cdb->getUrl("MBD_TQ_T0"); | ||
| if (Verbosity() > 0) | ||
| { | ||
| std::cout << "tq_t0_url " << tq_t0_url << std::endl; | ||
| std::cout << "MBD_TQ_T0 url " << _cdb_urls["MBD_TQ_T0"] << std::endl; | ||
| } | ||
| Download_TQT0(tq_t0_url); | ||
| Download_TQT0(_cdb_urls["MBD_TQ_T0"]); |
Contributor
There was a problem hiding this comment.
Keep QFIT and both T0 downloads outside _fitsonly.
The CDB path now skips MBD_QFIT, MBD_TT_T0, and MBD_TQ_T0 when _fitsonly is set, but the local-file path below still loads those three calibrations unconditionally. That makes physics behavior depend on where the calibrations came from and leaves _qfit_*, _ttfit_*, and _tqfit_* at reset values in fits-only CDB jobs.
Proposed fix
- if ( !_fitsonly )
- {
- _cdb_urls["MBD_QFIT"] = _cdb->getUrl("MBD_QFIT");
- if (Verbosity() > 0)
- {
- std::cout << "MBD_QFIT url " << _cdb_urls["MBD_QFIT"] << std::endl;
- }
- Download_Gains(_cdb_urls["MBD_QFIT"]);
-
- _cdb_urls["MBD_TT_T0"] = _cdb->getUrl("MBD_TT_T0");
- if ( Verbosity() > 0 )
- {
- std::cout << "MBD_TT_T0 url " << _cdb_urls["MBD_TT_T0"] << std::endl;
- }
- Download_TTT0(_cdb_urls["MBD_TT_T0"]);
-
- _cdb_urls["MBD_TQ_T0"] = _cdb->getUrl("MBD_TQ_T0");
- if (Verbosity() > 0)
- {
- std::cout << "MBD_TQ_T0 url " << _cdb_urls["MBD_TQ_T0"] << std::endl;
- }
- Download_TQT0(_cdb_urls["MBD_TQ_T0"]);
+ _cdb_urls["MBD_QFIT"] = _cdb->getUrl("MBD_QFIT");
+ if (Verbosity() > 0)
+ {
+ std::cout << "MBD_QFIT url " << _cdb_urls["MBD_QFIT"] << std::endl;
+ }
+ Download_Gains(_cdb_urls["MBD_QFIT"]);
+
+ _cdb_urls["MBD_TT_T0"] = _cdb->getUrl("MBD_TT_T0");
+ if ( Verbosity() > 0 )
+ {
+ std::cout << "MBD_TT_T0 url " << _cdb_urls["MBD_TT_T0"] << std::endl;
+ }
+ Download_TTT0(_cdb_urls["MBD_TT_T0"]);
+
+ _cdb_urls["MBD_TQ_T0"] = _cdb->getUrl("MBD_TQ_T0");
+ if (Verbosity() > 0)
+ {
+ std::cout << "MBD_TQ_T0 url " << _cdb_urls["MBD_TQ_T0"] << std::endl;
+ }
+ Download_TQT0(_cdb_urls["MBD_TQ_T0"]);
+
+ if ( !_fitsonly )
+ {
_cdb_urls["MBD_T0CORR"] = _cdb->getUrl("MBD_T0CORR");
if ( Verbosity() > 0 )
{
std::cout << "MBD_T0CORR url " << _cdb_urls["MBD_T0CORR"] << std::endl;
}
Build & test reportReport for commit 171fbeb3ee3ea6c66b1216c1d7cc3b7b0fdd8526:
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: added some helper methods
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)
MbdCalib Helper Methods for Calibration I/O and URL Management
Motivation & Context
This PR extends the MBD (Midrapidity Barrel Detector) calibration infrastructure by adding utility methods for slew correction file I/O and tracking of CDB (Conditions Database) URLs. These methods support offline calibration workflows and improve visibility into calibration data source tracking.
Key Changes
Write_SlewCorr()method: Serializes slew correction calibration data (_scorr_npts,_scorr_minrange,_scorr_maxrange,_scorr_yarrays) to a custom text file format. Follows the existing pattern of other Write_* methods in MbdCalib.Save_CDB_URL()method: Persists all CDB URLs stored in the_cdb_urlsmap as ROOTTNamedobjects, enabling URL tracking in ROOT output files for debugging/auditing purposes.Download_All(): Now centralizes CDB URL retrieval in the_cdb_urlsmap via_cdb->getUrl()calls, improving URL bookkeeping and added verbosity output for URL logging._cdb_urls:std::map<std::string, std::string>stores calibration type → URL mappings (e.g., "MBD_STATUS" → URL)._rawdstflagcomment in MbdEvent.h.Potential Risk Areas
Write_SlewCorr()uses a custom text serialization format. Compatibility depends on downstream consumers of this format; verify parsing expectations.Save_CDB_URL()relies on TNamed serialization; may have ROOT version compatibility considerations._cdb_urlsmap stores multiple URL strings; impact is negligible unless URL count or length grows significantly.#ifndef ONLINEprotected, so online reconstruction paths are unaffected.Implementation Notes
#ifndef ONLINE, isolating changes to offline-only builds.Write_SlewCorr()matches expected calibration file layouts, and that URL persistence in ROOT doesn't introduce unintended side effects.Possible Future Improvements
Write_SlewCorr()output._cdb_urlsto expose URL information programmatically.