Fixes for HCAL premixing #16057

kpedro88 · 2016-09-30T19:39:07Z

The first two commits in this PR fix regressions that were unknowingly introduced before pre12 (which prevent premixing from running at all):

Premixing config for new packer
Packing and unpacking presamples value for QIE8 in new packer

The second two commits fix known issues specifically for QIE11 premixing:
3) Packing and unpacking QIE11 flag word (used for premixing error bits)
4) Disabling SiPM dark current noise during premixing step 1

The following plots compare the QIE11 ADC distribution with standard mixing (blue) vs. premixing (red).

After commits 1,2:

After commits 1,2,3: (better agreement at high ADC where error bits are used)

After commits 1,2,3,4: (better agreement at low ADC where single pe noise was previously duplicated)

attn: @jmmans, @lihux25, @mdhildreth

cmsbuild · 2016-09-30T19:39:22Z

A new Pull Request was created by @kpedro88 (Kevin Pedro) for CMSSW_8_1_X.

It involves the following packages:

Configuration/StandardSequences
EventFilter/HcalRawToDigi
SimCalorimetry/HcalSimAlgos
SimCalorimetry/HcalSimProducers

@civanch, @cvuosalo, @mdhildreth, @cmsbuild, @franzoni, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @makortel, @GiacomoSguazzoni, @rovere, @VinInn, @Martin-Grunewald, @mariadalfonso, @dgulhan this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

kpedro88 · 2016-09-30T19:39:37Z

@cmsbuild please test

cmsbuild · 2016-09-30T19:39:49Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/15480/console

cmsbuild · 2016-09-30T21:15:53Z

+1
Tested at: 8296b7b
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16057/15480/summary.html

cmsbuild · 2016-09-30T22:23:49Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16057/15480/summary.html

kpedro88 · 2016-10-04T14:18:15Z

@slava77 @cvuosalo @civanch @mdhildreth please sign

mdhildreth · 2016-10-04T15:35:35Z

+1

slava77 · 2016-10-05T15:13:23Z

EventFilter/HcalRawToDigi/plugins/PackerHelp.h

@@ -394,13 +396,13 @@ class UHTRpacker{
     return header;
  }

-  uhtrData* newUHTR( int uhtrIndex , int orn = 0 , int bcn = 0 , uint64_t evt = 0 ){
+  uhtrData* newUHTR( int uhtrIndex , int ps = 0, bool specialSimPremixBit = false, int orn = 0 , int bcn = 0 , uint64_t evt = 0 ){


I see that all use cases have the second argument specified.
This looks like a good reason to remove the default value.

kpedro88 · 2016-10-05T16:37:02Z

@slava77 any more code review?

slava77 · 2016-10-05T16:42:20Z

EventFilter/HcalRawToDigi/plugins/PackerHelp.h

@@ -508,7 +516,7 @@ class UHTRpacker{
    HcalElectronicsId eid(readoutMap->lookup(detid));
    // loop over words in dataframe
    for(edm::DataFrame::iterator dfi=qiedf.begin() ; dfi!=qiedf.end(); ++dfi){      
-      if( dfi >= qiedf.end()-QIE11DataFrame::FLAG_WORDS ){
+      if( dfi >= qiedf.end() ){ //include flag word


I see changes in the HcalNoise plots. Is this the place in this PR that drives the changes?

This just packs the flag word from the sim digi so it can be unpacked later. The flag word is only set when running premixing, so it should not cause any changes.

I think this is the reason for the change in unmapped digis.

This is only called for QIE11, though, and the unmapped digis are all QIE8.

cmsbuild · 2016-10-07T00:13:41Z

+1
Tested at: f09f481
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16057/15583/summary.html

cmsbuild · 2016-10-07T01:39:40Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16057/15583/summary.html

slava77 · 2016-10-07T05:23:47Z

a new difference popped up in testing of the last changes:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_8_1_X_2016-10-06-1100+16057/16250/validateJR/all_OldVSNew_TTbar13TeV2017HCALdevwf10624p0/all_OldVSNew_TTbar13TeV2017HCALdevwf10624p0c_HcalUnpackerReport_hcalDigis__RECO_obj_unmappedDigis.png

HcalUnpackerReport for hcalDigis has a different size of unmappedDigis

The new test is done with a new GT:
https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/gts/81X_upgrade2017_HCALdev_v2/81X_upgrade2017_HCALdev_Candidate_2016_10_05_11_20_17

Which part of the packer/unpacker changed in this PR leads to this change in the unmappedDigis set?

abdoulline · 2016-10-07T05:46:15Z

~ "The morning is wiser than the evening".
Now I see... the new unpacked default was incorrectly set to default 10.

Concerning unpapped digis (why do we have any in 2017dev workflow at all once we use consistent 2017dev emap?), Hongxuan @lihux25 may want to comment as the new packer developer ?

abdoulline · 2016-10-07T06:22:10Z

Concerning maxZeros change when N_presamples are fixed - Halil @dertexaner
might want to comment.
This variable reflects the number of Digis with ADC zeros above some limit
http://cmslxr.fnal.gov/source/RecoMET/METProducers/src/HcalNoiseInfoProducer.cc#0411

Frankly, I don't see a simple ("linear") relation between wrong N_presamples in new MC packer and maxZeros (based on checking each ADC count in Digis). But there might be some (impact).

slava77 · 2016-10-07T16:55:28Z

signoff of this PR is pending explanations in observed changes

maxZeros and other noise variables are probably connected to eraw changes
unmapped digis change is also unexpected, apparently

dertexaner · 2016-10-07T17:36:37Z

RechitR45 and HPD noise filters rely on HBHE eraw (M0), so any change in M0 is likely to affect these filters as well.
MaxZeros (ADC0) filter is no longer in use since mid-2015. But it also has some minimum RBX energy threshold (10 GeV, probably calculated with M0 as well) so it is also plausible that maxZeros distributions get affected by changes to M0 since this could change the number of qualifying RBXs.

kpedro88 · 2016-10-07T18:03:51Z

@dertexaner thanks for the info. According to @igv4321 the presample info is used for method 0 (I think), so seems to be a plausible explanation. I will try to check the eraw values explicitly to see if this is reflected. (They're currently not tracked in validation/DQM, which should probably change...)

I'll also try to figure out what's up with the unmapped digis; a change there seems very odd.

kpedro88 · 2016-10-08T03:38:58Z

Updates:

When I check out a clean IB CMSSW_8_1_X_2016-09-29-2300 (current basis of this PR) and run 10624.0, eraw() for HB RecHits is 0. So I think this explains the changes we see in the noise plots and demonstrates that restoring the proper presample value is indeed a bug fix. (Checking eraw() with this PR shows the expected distribution.)
I added printouts everywhere report.countUnmappedDigi(eid) is called, in a clean IB CMSSW_8_1_X_2016-10-07-2300 and in the same IB plus this PR. Somehow the list of unmapped QIE8 UTCA electronics IDs does change with the PR included. I really don't know why this would be; the only change for QIE8 should be properly packing the presample value. @lihux25 @jmmans can you comment on this at all?

slava77 · 2016-10-08T03:50:28Z

On 10/7/16 8:39 PM, Kevin Pedro wrote:

Updates:

When I check out a clean IB |CMSSW_8_1_X_2016-09-29-2300| (current
basis of this PR) and run 10624.0, |eraw()| for HB RecHits is 0. So I
think this explains the changes we see in the noise plots and
demonstrates that restoring the proper presample value is indeed a bug
fix. (Checking |eraw()| with this PR shows the expected distribution.)

Great. This is an obvious improvement then.

I added printouts everywhere |report.countUnmappedDigi(eid)| is
called, in a clean IB |CMSSW_8_1_X_2016-10-07-2300| and in the same IB
plus this PR. Somehow the list of unmapped QIE8 UTCA electronics IDs
does change with the PR included. I really don't know why this would be;
the only change for QIE8 should be properly packing the presample value.
@lihux25 https://github.com/lihux25 @jmmans
https://github.com/jmmans can you comment on this at all?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#16057 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AEdcbk1ASMxkEuC3L-rGHTU4CXSLCInQks5qxxBUgaJpZM4KLcLR.

slava77 · 2016-10-08T19:33:32Z

Just dumping the unmapped raw IDs (dumping for 2K events from my pion sample):

the same set of IDs is present in both cases (144 kinds). There are more unmapped IDs with this PR.
all appear to be uTCA, at least for one event that I all are 0x40XXXXXX
- fiberChanId is 3
- fiberIndex is 31 (0x1F)

... based on finalizeHeadTail method, it looks like these are padding headers 0xD07F (looks like this is recognized as flavor 5, channel 3 fiber 31).
If I'm not mistaken, the changes here https://github.com/cms-sw/cmssw/pull/16057/files#diff-6929ef99c19e1aac98c2c67529f73efeR519 affect the length of uhtr data and by that the padding size.
So, we get a different number of "unmapped" digis

kpedro88 · 2016-10-08T19:35:21Z

@slava77 ah, I was not considering the padding size. You are probably right; including the QIE11 flag word would change the overall size of the raw data stream.

slava77 · 2016-10-08T19:38:44Z

+1

for #16057 f09f481

changes are in line with the description and the follow up review. As mentioned above, the main effect is the fix of presample value for QIE8, which recovers correct values for M0 energy reco in HB (from 0)
jenkins tests pass and comparisons with baseline show differences in noise flags, which depend on eraw.
- There is also a change in the count of unmapped digis, which apparently come out from (still unpacked) empty padding headers. These should eventually be detected and ignored.

kpedro88 · 2016-10-08T19:39:42Z

@mdhildreth @civanch please sign again

lihux25 · 2016-10-09T17:29:59Z

@slava77 and @kpedro88 thanks for catching the unmapped IDs. We do need padding headers when the word is not 64 bit for QIE8. After talking to Jeremey, he suggested a better way to assign this kind of header. I do plan to have a followup update once this PR is merged.

You are also right that the unmapped headers have no physical impact for now (would be better after following Jeremey's suggest in a later PR).

civanch · 2016-10-09T18:37:26Z

+1

kpedro88 · 2016-10-09T20:11:12Z

@davidlange6 can this be considered for pre13?

kpedro88 added 4 commits September 30, 2016 14:18

fix premix config

20f030c

keep presamples for qie8 in new packer

ec08635

store QIE11 flag word for HE premixing

14f8a01

disable SiPM dark current noise during premixing step 1

8296b7b

cmsbuild added this to the Next CMSSW_8_1_X milestone Sep 30, 2016

cmsbuild added comparison-pending operations-pending orp-pending pending-signatures reconstruction-pending simulation-pending tests-pending labels Sep 30, 2016

cmsbuild added tests-started and removed tests-pending labels Sep 30, 2016

cmsbuild added tests-approved and removed tests-started labels Sep 30, 2016

cmsbuild added comparison-available and removed comparison-pending labels Sep 30, 2016

cmsbuild added simulation-approved and removed simulation-pending labels Oct 4, 2016

slava77 reviewed Oct 5, 2016

View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Oct 6, 2016

cmsbuild added tests-approved and removed tests-started labels Oct 7, 2016

cmsbuild added comparison-available and removed comparison-pending labels Oct 7, 2016

slava77 mentioned this pull request Oct 7, 2016

HBHE: siPM M2 reco fixes #15959

Merged

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Oct 8, 2016

cmsbuild added simulation-approved and removed simulation-pending labels Oct 9, 2016

davidlange6 merged commit e8c4d47 into cms-sw:CMSSW_8_1_X Oct 9, 2016

kpedro88 mentioned this pull request Mar 14, 2017

Handle SiPM nonlinearity in premixing #17920

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for HCAL premixing #16057

Fixes for HCAL premixing #16057

kpedro88 commented Sep 30, 2016

cmsbuild commented Sep 30, 2016

kpedro88 commented Sep 30, 2016

cmsbuild commented Sep 30, 2016 •

edited

cmsbuild commented Sep 30, 2016

cmsbuild commented Sep 30, 2016

kpedro88 commented Oct 4, 2016

mdhildreth commented Oct 4, 2016

slava77 Oct 5, 2016

kpedro88 commented Oct 5, 2016

slava77 Oct 5, 2016

kpedro88 Oct 5, 2016

slava77 Oct 8, 2016

kpedro88 Oct 8, 2016

cmsbuild commented Oct 7, 2016

cmsbuild commented Oct 7, 2016

slava77 commented Oct 7, 2016

abdoulline commented Oct 7, 2016 •

edited

abdoulline commented Oct 7, 2016

slava77 commented Oct 7, 2016

dertexaner commented Oct 7, 2016

kpedro88 commented Oct 7, 2016

kpedro88 commented Oct 8, 2016

slava77 commented Oct 8, 2016

slava77 commented Oct 8, 2016

kpedro88 commented Oct 8, 2016

slava77 commented Oct 8, 2016

kpedro88 commented Oct 8, 2016

lihux25 commented Oct 9, 2016

civanch commented Oct 9, 2016

kpedro88 commented Oct 9, 2016

Fixes for HCAL premixing #16057

Fixes for HCAL premixing #16057

Conversation

kpedro88 commented Sep 30, 2016

cmsbuild commented Sep 30, 2016

kpedro88 commented Sep 30, 2016

cmsbuild commented Sep 30, 2016 • edited

cmsbuild commented Sep 30, 2016

cmsbuild commented Sep 30, 2016

kpedro88 commented Oct 4, 2016

mdhildreth commented Oct 4, 2016

slava77 Oct 5, 2016

Choose a reason for hiding this comment

kpedro88 commented Oct 5, 2016

slava77 Oct 5, 2016

Choose a reason for hiding this comment

kpedro88 Oct 5, 2016

Choose a reason for hiding this comment

slava77 Oct 8, 2016

Choose a reason for hiding this comment

kpedro88 Oct 8, 2016

Choose a reason for hiding this comment

cmsbuild commented Oct 7, 2016

cmsbuild commented Oct 7, 2016

slava77 commented Oct 7, 2016

abdoulline commented Oct 7, 2016 • edited

abdoulline commented Oct 7, 2016

slava77 commented Oct 7, 2016

dertexaner commented Oct 7, 2016

kpedro88 commented Oct 7, 2016

kpedro88 commented Oct 8, 2016

slava77 commented Oct 8, 2016

slava77 commented Oct 8, 2016

kpedro88 commented Oct 8, 2016

slava77 commented Oct 8, 2016

kpedro88 commented Oct 8, 2016

lihux25 commented Oct 9, 2016

civanch commented Oct 9, 2016

kpedro88 commented Oct 9, 2016

cmsbuild commented Sep 30, 2016 •

edited

abdoulline commented Oct 7, 2016 •

edited