Update DT vdrift calibration code to allow using "new" DB format #31808
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31808/19104
|
|
A new Pull Request was created by @namapane (Nicola Amapane) for master. It involves the following packages: CalibMuon/DTCalibration @yuanchao, @christopheralanwest, @tocheng, @cmsbuild, @tlampen, @pohsun can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins.
|
|
+1 |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 |
cmsbuild
added
alca-approved
fully-signed
and removed
alca-pending
pending-signatures
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
There was a problem hiding this comment.
I've added some comments about the migration to the event setup token https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToGetDataFromES#Getting_data_from_EventSetup_wit .
| mTimeMap_ = &*mTime; | ||
| if (readLegacyVDriftDB) { | ||
| ESHandle<DTMtime> mTime; | ||
| setup.get<DTMtimeRcd>().get(mTime); |
There was a problem hiding this comment.
please move from edm::EventSetup::get to edm::EventSetup::getData
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToGetDataFromES#Getting_data_from_EventSetup_wit
There was a problem hiding this comment.
OK, but this migration has to be done on these classes anyhow, it is not needed as a consequence of this PR.
Therefore it would make more sense to make it in its own PR, after this one is merged.
I can make a new PR for the migration right away after this one is merged; this is my preference given that this PR is by now tested and fully signed.
| mTimeMap_ = &*mTime; | ||
| } else { | ||
| ESHandle<DTRecoConditions> hVdrift; | ||
| setup.get<DTRecoConditionsVdriftRcd>().get(hVdrift); |
| // Get the map of vdrift from the Setup | ||
| if (readLegacyVDriftDB) { | ||
| ESHandle<DTMtime> mTime; | ||
| setup.get<DTMtimeRcd>().get(mTime); |
| mTimeMap_ = &*mTime; | ||
| } else { | ||
| ESHandle<DTRecoConditions> hVdrift; | ||
| setup.get<DTRecoConditionsVdriftRcd>().get(hVdrift); |
| if (version != 1) { | ||
| throw cms::Exception("Configuration") << "only version 1 is presently supported for VDriftDB"; | ||
| } | ||
| } | ||
|
|
||
| // Get geometry from Event Setup | ||
| setup.get<MuonGeometryRecord>().get(dtGeom_); |
There was a problem hiding this comment.
Besides, the same migration would have to be done everywhere in the package; for example. also in
https://github.com/cms-sw/cmssw/blob/master/CalibMuon/DTCalibration/plugins/DTTTrigCorrection.cc#L49
an generally in each and every cc under plugins, not just in those interested by this PR.
IMO this is another reason why that migration deserves its own, separate PR.
@silviodonato can you please let me know if you agree.
|
+1 |
PR description:
New DT payload formats were introduced with #5977, but so far have been adopted only for uncertainties (#9883).
This PR includes some updates to DT vdrift calibration code/scripts to enable to optionally read/write constants in the new format.
The default behaviour is unchanged (ie use the legacy format); it is one step towards enabling a complete migration.
PR validation:
Code changes tested by @saghosh :