fix ES unpacker when the 2nd OptoRX is disabled, but the 1st and 3rd … #11613

cmkuo · 2015-10-02T03:15:08Z

This is to fix the problem when the second ES OptoRX is disabled, but the first and third OptoRX are enabled in the ES unpacker. The problem would cause the data in third OptoRX are not unpacked.

…are not

cmsbuild · 2015-10-02T03:16:10Z

A new Pull Request was created by @cmkuo for CMSSW_7_4_X.

fix ES unpacker when the 2nd OptoRX is disabled, but the 1st and 3rd

It involves the following packages:

EventFilter/ESRawToDigi

@cmsbuild, @cvuosalo, @slava77 can you please review it and eventually sign? Thanks.
@Martin-Grunewald this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

slava77 · 2015-10-02T11:33:49Z

@cmsbuild please test
@cmkuo is there a run/file to test this in data?

cmsbuild · 2015-10-02T11:36:38Z

The tests are being triggered in jenkins.

cmkuo · 2015-10-02T12:53:22Z

@slava77 run 256843 is one of the runs affected by the bug.
If you would like to test on single file, you can try root://eoscms//eos/cms/store/data/Run2015D/DoubleEG/RAW/v1/000/256/843/00000/00E5B2E5-AA5E-E511-B223-02163E0142DD.root

cmsbuild · 2015-10-02T13:17:59Z

The jenkins tests job failed, please try again.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/8585/console

slava77 · 2015-10-02T22:38:34Z

... I restarted the tests; let's see if they complete this time

cmsbuild · 2015-10-02T23:59:23Z

+1
Tested at: 5cfff3e
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11613/8595/summary.html

cmsbuild · 2015-10-03T00:49:07Z

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

cvuosalo · 2015-10-06T19:23:05Z

EventFilter/ESRawToDigi/src/ESUnpacker.cc

+  }
+  if (optoRX2_ == 128) {
+    enableOptoRX[NenableOptoRX] = 2;
+    NenableOptoRX++;


NenableOptoRX is not used after this line, so the increment would seem to serve no purpose. Please delete it.

cvuosalo · 2015-10-06T19:57:13Z

@cmkuo:
I ran prompt reco on the file you suggested with and without your fix. What's an easy way to tell from the output files that your fix worked correctly?

slava77 · 2015-10-06T20:35:15Z

On 10/6/15 2:57 PM, Carl Vuosalo wrote:

@cmkuo https://github.com/cmkuo:
I ran prompt reco on the file you suggested with and without your fix.
What's an easy way to tell from the output files that your fix worked
correctly?

I would think that DQM plots will change, probably even the fwlite plots
as well.
Carl, do you see any difference at all?

—
Reply to this email directly or view it on GitHub
#11613 (comment).

cvuosalo · 2015-10-06T22:17:51Z

@slava77:
Yes, I found tiny differences in the fwlite plots for ECAL preshower rechit , PF candidate, PF MET, and ak4PFjet quantities. Also, these product size differences show up:

-------------------------------------------------------------
 or, B       new, B      delta, B   delta, %   deltaJ, %    branch 
-------------------------------------------------------------
   122054 ->      122781        727      0.6   0.06     recoPFClusters_particleFlowClusterPS__RECO.
    44236 ->       44368        132      0.3   0.01     TrackingRecHitsOwned_generalTracks__RECO.
    86458 ->       86985        528      0.6   0.04     EcalRecHitsSorted_ecalPreshowerRecHit_EcalRecHitsES_RECO.
-------------------------------------------------------------
  1291104 ->     1292600       1496             0.1     ALL BRANCHES

It looks like the missing OptoRX data has been restored, so I think the PR is working correctly. After the code fixes are made and new Jenkins tests are passed, Reco should be able to approve this PR.

slava77 · 2015-10-06T22:35:36Z

I expect that some 2D occupancy plots in DQM will be quite clear about
the fix
(localized to the problematic area of ES)

On 10/6/15 5:17 PM, Carl Vuosalo wrote:

@slava77 https://github.com/slava77:
Yes, I found tiny differences in the fwlite plots for ECAL preshower
rechit , PF candidate, PF MET, and ak4PFjet quantities. Also, these
product size differences show up:

|------------------------------------------------------------- or, B new,
B delta, B delta, % deltaJ, % branch
------------------------------------------------------------- 122054 ->
122781 727 0.6 0.06 recoPFClusters_particleFlowClusterPS__RECO. 44236 ->
44368 132 0.3 0.01 TrackingRecHitsOwned_generalTracks__RECO. 86458 ->
86985 528 0.6 0.04
EcalRecHitsSorted_ecalPreshowerRecHit_EcalRecHitsES_RECO.
------------------------------------------------------------- 1291104 ->
1292600 1496 0.1 ALL BRANCHES |

It looks like the missing OptoRX data has been restored, so I think the
PR is working correctly. After the code fixes are made and new Jenkins
tests are passed, Reco should be able to approve this PR.

—
Reply to this email directly or view it on GitHub
#11613 (comment).

cmsbuild · 2015-10-08T14:36:25Z

Pull request #11613 was updated. @cmsbuild, @cvuosalo, @slava77 can you please check and sign again.

slava77 · 2015-10-08T15:20:45Z

@cmkuo we will need similar updates in 75X and 76X.
Please update existing PRs (#11615 in 75X) or create a new one (in 76X)
Thank you.

cmkuo · 2015-10-08T15:34:22Z

@slava77
#11615 for 75X is updated
Somehow I cannot update 76X one so I create a new PR
#11693

slava77 · 2015-10-08T15:38:01Z

@cmsbuild please test

cmsbuild · 2015-10-08T15:42:27Z

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

cmsbuild · 2015-10-08T17:42:36Z

+1
Tested at: 545e2b5
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11613/8699/summary.html

cmsbuild · 2015-10-08T18:38:08Z

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

slava77 · 2015-10-08T20:28:33Z

+1

for #11613 545e2b5

compared to the previous signoff, the code review comments were addressed. The same updates were applied here 5cfff3e...545e2b5 as in 75X and in 76X improve ES unpacker #11693
jenkins tests pass and comparisons with the baseline show no differences

@cvuosalo is offline today, I took the liberty to sign off in case there is an early 74X build

cmsbuild · 2015-10-08T20:31:49Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_4_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_7_6_X is complete. This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar

davidlange6 · 2015-10-09T14:52:23Z

+1

fix ES unpacker when the 2nd OptoRX is disabled, but the 1st and 3rd …

fix ES unpacker when the 2nd OptoRX is disabled, but the 1st and 3rd …

5cfff3e

…are not

cmsbuild added this to the Next CMSSW_7_4_X milestone Oct 2, 2015

cmsbuild added reconstruction-pending pending-signatures tests-pending orp-pending comparison-pending labels Oct 2, 2015

cmsbuild added tests-started and removed tests-pending labels Oct 2, 2015

cmsbuild added tests-pending and removed tests-started labels Oct 2, 2015

cmsbuild added tests-approved and removed tests-pending labels Oct 3, 2015

cmsbuild added comparison-available and removed comparison-pending labels Oct 3, 2015

cvuosalo reviewed Oct 6, 2015
View reviewed changes

cmsbuild added reconstruction-pending pending-signatures tests-pending comparison-pending and removed tests-approved labels Oct 8, 2015

cmkuo mentioned this pull request Oct 8, 2015

improve ES unpacker #11693

Merged

cmsbuild added tests-started and removed tests-pending labels Oct 8, 2015

cmsbuild added tests-approved and removed tests-started labels Oct 8, 2015

cmsbuild added comparison-available and removed comparison-pending labels Oct 8, 2015

cmsbuild added reconstruction-approved fully-signed and removed pending-signatures reconstruction-pending labels Oct 8, 2015

cmsbuild added orp-approved and removed orp-pending labels Oct 9, 2015

cmsbuild added a commit that referenced this pull request Oct 9, 2015

Merge pull request #11613 from cmkuo/mydev_esunpacker

a8c6346

fix ES unpacker when the 2nd OptoRX is disabled, but the 1st and 3rd …

cmsbuild merged commit a8c6346 into cms-sw:CMSSW_7_4_X Oct 9, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix ES unpacker when the 2nd OptoRX is disabled, but the 1st and 3rd … #11613

fix ES unpacker when the 2nd OptoRX is disabled, but the 1st and 3rd … #11613

cmkuo commented Oct 2, 2015

cmsbuild commented Oct 2, 2015

slava77 commented Oct 2, 2015

cmsbuild commented Oct 2, 2015

cmkuo commented Oct 2, 2015

cmsbuild commented Oct 2, 2015

slava77 commented Oct 2, 2015

cmsbuild commented Oct 2, 2015

cmsbuild commented Oct 3, 2015

cvuosalo Oct 6, 2015

cvuosalo commented Oct 6, 2015

slava77 commented Oct 6, 2015

cvuosalo commented Oct 6, 2015

slava77 commented Oct 6, 2015

cmsbuild commented Oct 8, 2015

slava77 commented Oct 8, 2015

cmkuo commented Oct 8, 2015

slava77 commented Oct 8, 2015

cmsbuild commented Oct 8, 2015

cmsbuild commented Oct 8, 2015

cmsbuild commented Oct 8, 2015

slava77 commented Oct 8, 2015

cmsbuild commented Oct 8, 2015

davidlange6 commented Oct 9, 2015

fix ES unpacker when the 2nd OptoRX is disabled, but the 1st and 3rd … #11613

fix ES unpacker when the 2nd OptoRX is disabled, but the 1st and 3rd … #11613

Conversation

cmkuo commented Oct 2, 2015

cmsbuild commented Oct 2, 2015

slava77 commented Oct 2, 2015

cmsbuild commented Oct 2, 2015

cmkuo commented Oct 2, 2015

cmsbuild commented Oct 2, 2015

slava77 commented Oct 2, 2015

cmsbuild commented Oct 2, 2015

cmsbuild commented Oct 3, 2015

cvuosalo Oct 6, 2015

Choose a reason for hiding this comment

cvuosalo commented Oct 6, 2015

slava77 commented Oct 6, 2015

cvuosalo commented Oct 6, 2015

slava77 commented Oct 6, 2015

cmsbuild commented Oct 8, 2015

slava77 commented Oct 8, 2015

cmkuo commented Oct 8, 2015

slava77 commented Oct 8, 2015

cmsbuild commented Oct 8, 2015

cmsbuild commented Oct 8, 2015

cmsbuild commented Oct 8, 2015

slava77 commented Oct 8, 2015

cmsbuild commented Oct 8, 2015

davidlange6 commented Oct 9, 2015