add HCAL laser monitoring charge #21152
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks |
|
A new Pull Request was created by @jkunkle for master. It involves the following packages: DataFormats/METReco @perrotta, @cmsbuild, @slava77 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 Tested at: 7679281 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: You can see the results of the tests here: I found follow errors while testing this PR Failed tests: RelVals AddOn
When I ran the RelVals I found an error in the following worklfows: runTheMatrix-results/7.3_CosmicsSPLoose_UP17+CosmicsSPLoose_UP17+DIGICOS_UP17+RECOCOS_UP17+ALCACOS_UP17+HARVESTCOS_UP17/step2_CosmicsSPLoose_UP17+CosmicsSPLoose_UP17+DIGICOS_UP17+RECOCOS_UP17+ALCACOS_UP17+HARVESTCOS_UP17.log136.788 step2 runTheMatrix-results/136.788_RunSinglePh2017B+RunSinglePh2017B+HLTDR2_2017+RECODR2_2017reHLT_skimSinglePh_Prompt+HARVEST2017/step2_RunSinglePh2017B+RunSinglePh2017B+HLTDR2_2017+RECODR2_2017reHLT_skimSinglePh_Prompt+HARVEST2017.log10042.0 step2 runTheMatrix-results/10042.0_ZMM_13+ZMM_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017/step2_ZMM_13+ZMM_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017.log10024.0 step2 runTheMatrix-results/10024.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017/step2_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017.log10224.0 step2 runTheMatrix-results/10224.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017PU_GenSimFull+DigiFullPU_2017PU+RecoFullPU_2017PU+HARVESTFullPU_2017PU/step2_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017PU_GenSimFull+DigiFullPU_2017PU+RecoFullPU_2017PU+HARVESTFullPU_2017PU.log11624.0 step2 runTheMatrix-results/11624.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2019_GenSimFull+DigiFull_2019+RecoFull_2019+ALCAFull_2019+HARVESTFull_2019/step2_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2019_GenSimFull+DigiFull_2019+RecoFull_2019+ALCAFull_2019+HARVESTFull_2019.log10824.0 step2 runTheMatrix-results/10824.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2018_GenSimFull+DigiFull_2018+RecoFull_2018+ALCAFull_2018+HARVESTFull_2018/step2_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2018_GenSimFull+DigiFull_2018+RecoFull_2018+ALCAFull_2018+HARVESTFull_2018.log
I found errors in the following addon tests: cmsRun /cvmfs/cms-ib.cern.ch/nweek-02496/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_10_0_X_2017-11-03-1100/src/HLTrigger/Configuration/test/OnLine_HLT_2e34v31.py realData=False globalTag=@ inputFiles=@ : FAILED - time: date Fri Nov 3 22:31:46 2017-date Fri Nov 3 22:21:38 2017 s - exit: 18688 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped) |
cmsbuild
added
comparison-notrun
tests-rejected
and removed
comparison-pending
tests-started
labels
|
email with a (fixing) recipe sent to internal HCAL thread... |
There was a problem hiding this comment.
While applying the fix suggested in #21152 (comment), please take also into account the following (preliminary) review comments
| @@ -133,7 +133,7 @@ | |||
| <version ClassVersion="12" checksum="4280559065"/> | |||
| <version ClassVersion="13" checksum="382065414"/> | |||
| <version ClassVersion="14" checksum="651605939"/> | |||
| <version ClassVersion="15" checksum="2415916091"/> | |||
| <version ClassVersion="15" checksum="1886001321"/> | |||
There was a problem hiding this comment.
Please, put back the previous class version, and move to a new one ("16") because of the added class member
There was a problem hiding this comment.
checksum="2415916091" (if you want to restore the previous one)
| @@ -110,6 +111,7 @@ namespace reco { | |||
| edm::EDGetTokenT<reco::PFJetCollection> jet_token_; | |||
|
|
|||
| double TotalCalibCharge; // placeholder to calculate total charge in calibration channels | |||
| double TotalLasmonCharge; // placeholder to calculate total charge in laser monitor channels | |||
There was a problem hiding this comment.
Class members should start with lowercase letters, uppercase initials are reserved to class names
(Please to the same also for TotalCalibCharge, and other possible occurrencies in this class)
| @@ -130,8 +132,16 @@ namespace reco { | |||
|
|
|||
| uint32_t HcalAcceptSeverityLevel_; | |||
| std::vector<int> HcalRecHitFlagsToBeExcluded_; | |||
|
|
|||
| std::vector<int> LaserMonDetTypeList_; | |||
There was a problem hiding this comment.
Class members should start with lowercase letters, uppercase initials are reserved to class names
| std::vector<int> LaserMonIPhiList_; | ||
| std::vector<int> LaserMonIEtaList_; | ||
|
|
||
| int LaserMonitorTSStart_; |
There was a problem hiding this comment.
Class members should start with lowercase letters, uppercase initials are reserved to class names
| 0.0, 0.0, 1.0, -0.2) # equivalent to "energy fraction > 0.2" | ||
| 0.0, 0.0, 1.0, -0.2), # equivalent to "energy fraction > 0.2" | ||
|
|
||
| LaserMonDetTypeList = cms.vint32(3,3,3,3,3,3,3,3), |
There was a problem hiding this comment.
Lower case initials also for config parameters
| LaserMonIPhiList = cms.vint32(23,17,11,5,29,35,41,47), | ||
| LaserMonIEtaList = cms.vint32(0,0,0,0,0,0,0,0), | ||
|
|
||
| #LaserMonitorTSStart = cms.int32( 4 ), |
There was a problem hiding this comment.
Are these commented out parameters needed: if so, please add a comment explaining why you keep them commented out, otherwise just remove
| @@ -112,6 +113,49 @@ HcalNoiseInfoProducer::HcalNoiseInfoProducer(const edm::ParameterSet& iConfig) : | |||
| edm::LogWarning("HCalNoiseInfoProducer") << " forcing fillRecHits to be true if fillDigis is true.\n"; | |||
| } | |||
|
|
|||
| // get the fiber configuration vectors | |||
| std::vector<int> TmpLaserMonDetTypeList = iConfig.getParameter<std::vector<int> >("LaserMonDetTypeList"); | |||
There was a problem hiding this comment.
Local variables should start with lowercase letters, as well as the config parameters
|
|
||
| // now get the digis | ||
| int ts_size = int(digi->size()); | ||
| for(int i = 0; i < ts_size; i++) { |
| } | ||
|
|
||
| // get the integration region with defaults | ||
| if( iConfig.existsAs<int>("LaserMonitorTSStart" ) ) { |
There was a problem hiding this comment.
use of existsAs should be justified, because it is an exception.
Default values for parameters should be defined with fillDescriptions.
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideConfigurationValidationAndHelp
|
@jkunkle : please update about the status of the implementation of the bug fix mentioned in #21152 (comment), and the implementation of the preliminary review comments provided in #21152 (review) and #21152 (review) Could you please also post here a recipe for testing the effects of this PR, once fixed? |
|
@junkle : is there any update? Will this Pull Request get discussed by @mariadalfonso (or some other HCAL people) this week at the reco meeting? If not, maybe you could come yourself and discuss it: we can allow one slot for it, just let us know (and please contact me, perrotta@vxcern.cern.ch, to arrange for it) |
|
@perrotta Hi, someone appears to have accessed my account without my authorization. Please disregard any previous actions from it. |
cmsbuild
added
code-checks-pending
comparison-pending
tests-pending
and removed
tests-approved
labels
|
The code-checks are being triggered in jenkins. |
|
+code-checks |
|
The tests are being triggered in jenkins. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1
|
| int ieta = digi->id().ieta(); | ||
|
|
||
| // only check channels having the requested cboxch | ||
| if (std::find( laserMonCBoxList_.begin(), laserMonCBoxList_.end(), |
There was a problem hiding this comment.
just to clarify the previous discussion - "find" is effectively looping over your container - so its not that you are just looking at the 4-8 channels of interest, your looking at all of them (and 4-8 of them, you are looking twice)
There was a problem hiding this comment.
Right, there are two cases
For the channels of interest the std::find should return true on the first iteration and then perform the inner loop
For all other channels the std::find makes one loop and returns false
so yes, its clear that the std::find does not help, but it does not hurt much. I can remove it if you like
|
merge |
| @@ -194,6 +195,7 @@ class HcalNoiseSummary | |||
| double rechitEnergy_; | |||
| double rechitEnergy15_; | |||
| double calibCharge_; | |||
| double lasmonCharge_; | |||
There was a problem hiding this comment.
this variable is not initialized.
Since I've added the MET filter path monitoring a few days ago, now we find that this leads to random differences when reading AOD produced by a release before 100X.
E.g. #25320 (comment)
|
Thank you Slava!
@sarafiorendi
Slava Krutelyov <notifications@github.com> ha scritto:
… slava77 commented on this pull request.
> @@ -194,6 +195,7 @@ class HcalNoiseSummary
double rechitEnergy_;
double rechitEnergy15_;
double calibCharge_;
+ double lasmonCharge_;
this variable is not initialized.
Since I've added the MET filter path monitoring a few days ago, now
we find that this leads to random differences when reading AOD
produced by a release before 100X.
E.g. #25320 (comment)
@perrotta
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#21152 (review)
|
Added a float and a getter function to HcalNoiseSummary to store/retrieve the laser monitoring energy
Added code in HcalNoiseInfoProducer to calculate the charge and add it to the noise summary
The inputs to the calculation are provided in hcalnoiseinfoproducer_cfi.py :