Cleanup of DTRecoUncertainties/DTRecoUncertaintiesRcd #35208
Conversation
…DTRecoUncertainties
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35208/25155
|
|
A new Pull Request was created by @namapane (Nicola Amapane) for master. It involves the following packages:
@malbouis, @andrius-k, @yuanchao, @kmaeshima, @ErnestaP, @ahmad3213, @cmsbuild, @jfernan2, @rvenditti, @francescobrivio, @tvami can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| ESHandle<DTRecoConditions> h_rconds; | ||
| setup.get<DTRecoConditionsUncertRcd>().get(h_rconds); | ||
| rconds = &*h_rconds; |
There was a problem hiding this comment.
Hi @namapane since you are touching this part of the code, can you please move to the new method with the Tokens?
To use ESGetToken see
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToGetDataFromES#In_ED_module
To get data with the token see
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToGetDataFromES#Getting_data_from_EventSetup_wit
There was a problem hiding this comment.
OK but I may need to change the subject of the PR, since that will amount tomore changes than those targeted here :-)
There was a problem hiding this comment.
I'm also fine if it's done in a separate PR if you prefer that.
There was a problem hiding this comment.
That would be great (I started coding but I'd need a moment to test the change for all 7 objects that are consumed here)
| cms.PSet(record = cms.string("DTRecoUncertaintiesRcd"), | ||
| tag = cms.string("DTRecoUncertainties_test"), | ||
| cms.PSet(record = cms.string("DTRecoConditionsUncertRcd"), | ||
| tag = cms.string("UncertDB"), | ||
| connect = cms.untracked.string("sqlite_file:"+uncertDB)) |
There was a problem hiding this comment.
I'm not sure what's happening here, there is a local sqlite file that's read here? DTRecoConditionsUncertRcd is still kept in the GT, right? So why not just use what's in the GT instead of a specification here?
There was a problem hiding this comment.
This snippet (which is for local tests, i.e. for running manually for testing purposes) allows also, optionally, to take alternate configurations from a .db file (we sometime need to test stuff before we put it them in GTs...)
|
@cmsbuild , please test |
|
-1 Failed Tests: UnitTests The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
You can see more details here: Unit TestsI found errors in the following unit tests: ---> test TestCTPPSDirectProtonSimulation had ERRORS Comparison SummarySummary:
|
|
@cmsbuild , please test
|
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1c2853/18463/summary.html Comparison SummarySummary:
|
|
+alca
|
|
+1 |
cmsbuild
added
dqm-approved
fully-signed
and removed
dqm-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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
These DB formats have been superseeded since a few years.
This PR removes a few occurrences in test code (while definitions still remain in place)
Please note that the two .cc files involved are some test utilities that are intended to be run manually by experts to inspect payloads; a few couts are present and I think they are legitimate in this context.