Ecalpf cluster corrections93 x - taking care of events with flag==5 #19827

jainshilpi · 2017-07-19T15:57:35Z

This is taking care of events in which flag==5. These are forces ZS events. So these should go under the ZS category. Similarly, there can be events which have flag==7. Those are
forced FULL readout. So those should go under the FULL readout category. Code has been changed accordingly now.
@rafaellopesdesa @slava77

cmsbuild · 2017-07-19T15:57:53Z

A new Pull Request was created by @jainshilpi for master.

It involves the following packages:

RecoParticleFlow/PFClusterProducer

@perrotta, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@mmarionncern, @felicepantaleo, @rafaellopesdesa, @lgray, @seemasharmafnal, @cbernet, @bachtis this is something you requested to watch as well.
@davidlange6 you are the release manager for this.

cms-bot commands are listed here

slava77 · 2017-07-19T16:05:10Z

@cmsbuild please test

cmsbuild · 2017-07-19T16:05:28Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/21621/console Started: 2017/07/19 20:46

slava77 · 2017-07-19T16:06:45Z

@jainshilpi
if I understand correctly, the behavior will change after this PR.
Do these forced ZS or full readout appear in MC or is it just data?
If there are some slides or a document describing when/how the other bits can happen, it would be nice to know.

Thank you.

cmsbuild · 2017-07-19T19:52:59Z

+1
Tested at: 6f95120
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19827/21621/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
b5ed5c7
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19827/21621/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19827/21621/git-merge-result

cmsbuild · 2017-07-19T19:53:02Z

Comparison job queued.

cmsbuild · 2017-07-19T21:27:42Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 23
DQMHistoTests: Total histograms compared: 2310405
DQMHistoTests: Total failures: 15093
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2295146
DQMHistoTests: Total skipped: 166
DQMHistoTests: Total Missing objects: 0
Checked 93 log files, 14 edm output root files, 23 DQM output files

jainshilpi · 2017-07-19T22:04:47Z

@slava77 yes this is correct, the behaviour will change after this PR. earlier clusters which were forced
ZS were taken as in full readout. Now they will appropriately be taken in ZS. I will check if this can happen in MC as well. About the slides/documents as to how bits 5 and 7 can happen, I just know the pointer to this code:
https://github.com/cms-sw/cmssw/blob/master/DataFormats/EcalDigi/interface/EcalSrFlag.h
which says that /** Mask for the 'forced' bit which is set when the decision was

forced by an error condition or by configuration.
E.g., a force full readout flag has value SRF_FORCED_MASK|SRF_FULL

*/
I have also written to Andrea Massironi and Emanuele. I will let you know when I get more information about it.

Thanks

jainshilpi · 2017-07-20T09:09:02Z

@slava77 , so I checked with Emanuele. This cannot happen in MC.
And this is what Emanuele mentioned about the forced decisions:

"these are the correspondent “forced” bits of the Selective Readout flags (ZS or FR),
which occur when the DCC has some issue in applying the readout determined by the trigger (high/medium/low interest).
The force ZS flag can happen on data (not in MC) when the data payload of the Data Concentrator Card (DCC)
is too high, for example at the beginning of a high PU fill in the barrel, so the DAQ has to read also
the high interest zones in zero suppression mode. "

slava77 · 2017-07-25T18:20:21Z

RecoParticleFlow/PFClusterProducer/src/PFClusterEMEnergyCorrector.cc

-    if (clusFlag!=1 && clusFlag!=3) {
-      edm::LogWarning("PFClusterEMEnergyCorrector") << "We can only correct regions readout in ZS (flag 1) or FULL readout (flag 3). Flag " << clusFlag << " is not recognized."
+    if (!ZS_bit && !FR_bit) {
+      edm::LogWarning("PFClusterEMEnergyCorrector") << "We can only correct regions readout in ZS (flag 1,5) or FULL readout (flag 3,7). Flag " << clusFlag << " is not recognized."


as defined, this warning will not be made for clusFlag of 2,6, which apparently do not exist based on the description above.
should they be captured here as well?
if ( (!ZS_bit && !FR_bit) || (!ZS_bit && FR_bit) )

... which is actually just if ( !ZS_bit)

actually, values like above 7 should probably still have a warning as well
if ( !ZS_bit || clusFlag > 7)

slava77 · 2017-07-26T17:34:26Z

@jainshilpi
are you available to address #19827 (comment)
soon?
Please let me know.
Thank you.

slava77 · 2017-07-28T21:58:38Z

RecoParticleFlow/PFClusterProducer/src/PFClusterEMEnergyCorrector.cc

      if (pt<2.5) coridx = 1 + regind;
      else if (pt>=2.5 && pt<6.) coridx = 2 + regind;
      else if (pt>=6.) coridx = 3 + regind;
    }
-    if (clusFlag!=1 && clusFlag!=3) {
-      edm::LogWarning("PFClusterEMEnergyCorrector") << "We can only correct regions readout in ZS (flag 1) or FULL readout (flag 3). Flag " << clusFlag << " is not recognized."
+    if (ZS_bit!=0 || clusFlag > 7) {


here and above the change is made backwards for values of x in 0 or 1, !x is the same as x==0
instead of the suggested x!=0

@slava77 I realized I made a stupid mistake. Its corrected now. Thank you

slava77 · 2017-07-31T15:26:05Z

@jainshilpi
please follow up on the last update to fix the bug introduced.
Thank you.

cmsbuild · 2017-07-31T21:46:05Z

Pull request #19827 was updated. @perrotta, @cmsbuild, @slava77, @davidlange6 can you please check and sign again.

slava77 · 2017-07-31T21:47:13Z

@cmsbuild please test

cmsbuild · 2017-07-31T21:47:31Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/21934/console Started: 2017/07/31 23:48

cmsbuild · 2017-07-31T23:04:30Z

+1
Tested at: d067060
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19827/21934/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
e41a587
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19827/21934/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19827/21934/git-merge-result

cmsbuild · 2017-07-31T23:04:33Z

Comparison job queued.

cmsbuild · 2017-08-01T04:41:58Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 16 differences found in the comparisons
DQMHistoTests: Total files compared: 25
DQMHistoTests: Total histograms compared: 2651091
DQMHistoTests: Total failures: 61519
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2589391
DQMHistoTests: Total skipped: 181
DQMHistoTests: Total Missing objects: 0
Checked 102 log files, 14 edm output root files, 25 DQM output files

slava77 · 2017-08-01T05:10:21Z

+1

for #19827 d067060

technical changes were made since the last sign off in response to code review from @davidlange6 .
jenkins tests pass. The comparisons now show small difference in 2017 wf 136.788, which can happen in this setup and is in agreement with the earlier local tests reported in Ecalpf cluster corrections93 x - taking care of events with flag==5 #19827 (comment)

cmsbuild · 2017-08-01T05:10:41Z

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 requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar

davidlange6 · 2017-08-01T07:44:44Z

+1

jainshilpi added 2 commits July 19, 2017 17:27

Taken care of flag==5

bff41de

Taken care of flag==5

6f95120

cmsbuild added this to the CMSSW_9_3_X milestone Jul 19, 2017

cmsbuild added comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jul 19, 2017

cmsbuild added tests-started and removed tests-pending labels Jul 19, 2017

cmsbuild added tests-approved and removed tests-started labels Jul 19, 2017

cmsbuild added comparison-available and removed comparison-pending labels Jul 19, 2017

slava77 reviewed Jul 25, 2017

View reviewed changes

Taken care of flags 2,6 and > 7

f9ce512

cmsbuild added comparison-pending tests-pending and removed comparison-available tests-approved labels Jul 26, 2017

cmsbuild added the comparison-available label Jul 28, 2017

slava77 reviewed Jul 28, 2017

View reviewed changes

corrected the ZS and FR conditions - bug which was introduced earlier

d067060

cmsbuild added comparison-pending tests-pending and removed comparison-available tests-approved labels Jul 31, 2017

cmsbuild added tests-started and removed tests-pending labels Jul 31, 2017

cmsbuild added tests-approved and removed tests-started labels Jul 31, 2017

cmsbuild added comparison-available and removed comparison-pending labels Aug 1, 2017

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Aug 1, 2017

cmsbuild added orp-approved and removed orp-pending labels Aug 1, 2017

cmsbuild merged commit 3a437c2 into cms-sw:master Aug 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ecalpf cluster corrections93 x - taking care of events with flag==5 #19827

Ecalpf cluster corrections93 x - taking care of events with flag==5 #19827

jainshilpi commented Jul 19, 2017

cmsbuild commented Jul 19, 2017

slava77 commented Jul 19, 2017

cmsbuild commented Jul 19, 2017 •

edited

slava77 commented Jul 19, 2017

cmsbuild commented Jul 19, 2017

cmsbuild commented Jul 19, 2017

cmsbuild commented Jul 19, 2017

jainshilpi commented Jul 19, 2017

jainshilpi commented Jul 20, 2017

slava77 Jul 25, 2017

slava77 Jul 25, 2017

slava77 Jul 25, 2017

slava77 commented Jul 26, 2017

slava77 Jul 28, 2017

jainshilpi Jul 31, 2017

slava77 commented Jul 31, 2017

cmsbuild commented Jul 31, 2017

slava77 commented Jul 31, 2017

cmsbuild commented Jul 31, 2017 •

edited

cmsbuild commented Jul 31, 2017

cmsbuild commented Jul 31, 2017

cmsbuild commented Aug 1, 2017

slava77 commented Aug 1, 2017

cmsbuild commented Aug 1, 2017

davidlange6 commented Aug 1, 2017

Ecalpf cluster corrections93 x - taking care of events with flag==5 #19827

Ecalpf cluster corrections93 x - taking care of events with flag==5 #19827

Conversation

jainshilpi commented Jul 19, 2017

cmsbuild commented Jul 19, 2017

slava77 commented Jul 19, 2017

cmsbuild commented Jul 19, 2017 • edited

slava77 commented Jul 19, 2017

cmsbuild commented Jul 19, 2017

cmsbuild commented Jul 19, 2017

cmsbuild commented Jul 19, 2017

jainshilpi commented Jul 19, 2017

jainshilpi commented Jul 20, 2017

slava77 Jul 25, 2017

Choose a reason for hiding this comment

slava77 Jul 25, 2017

Choose a reason for hiding this comment

slava77 Jul 25, 2017

Choose a reason for hiding this comment

slava77 commented Jul 26, 2017

slava77 Jul 28, 2017

Choose a reason for hiding this comment

jainshilpi Jul 31, 2017

Choose a reason for hiding this comment

slava77 commented Jul 31, 2017

cmsbuild commented Jul 31, 2017

slava77 commented Jul 31, 2017

cmsbuild commented Jul 31, 2017 • edited

cmsbuild commented Jul 31, 2017

cmsbuild commented Jul 31, 2017

cmsbuild commented Aug 1, 2017

slava77 commented Aug 1, 2017

cmsbuild commented Aug 1, 2017

davidlange6 commented Aug 1, 2017

cmsbuild commented Jul 19, 2017 •

edited

cmsbuild commented Jul 31, 2017 •

edited