Restore fed error treatment for phase0 pixel detector #26679

veszpv · 2019-05-07T08:38:56Z

Resubmitting original PR26606 without the format checks.

PR description:

The FED raw data format for phase 0 and phase 1 detectors are essentially the same with small differences for the bit-packing of the pixel hit information in layer 1. There is a difference also in the error assignment by the front-end (*). The former case has been treated in the PixelDataFormatter by setting the 'phase1' variable through python configuration files. The error decoding class, called ErrorChecker, however was not made aware of the difference, instead, was just simply updated to phase 1.
This PR fixes that: a base class is defined for the ErrorChecker, the present default code is kept with the same name deriving from this base, and a new derived class ErrorCheckerPhase0 is introduced to restore the phase 0 treatment. The proper error checker is instantiated by the PixelDataFormatter based on the value of 'phase1', so no change needs to be made in the python configurations.

FED error 25: in phase 1, FED error type 25 has been reinterpreted and used by the FED firmware to signal "dead" links. It is used to flag stuck TBMs. This error has a totally different meaning in phase 0. In this PR, the special treatment of this error type by raw-to-digi and digi-to-raw is cancelled for phase 0.

(*) Few error codes have different interpretation; and in phase 0 FPix, the error association with detId is ambiguous.
PR validation:

During the reconstruction of phase 0 data, when a rare type 25 error was encountered, the tracking code crashed. This PR fixes that. The FED25 error packing in simulation, and unpacking in simulation and data for phase 1 should be unchanged, are being validated by @tsusa.

@tsusa @mmusich @dkotlins

…etector. An empty map results in no FED25 error being packed into the RAW data.

…of phase 0, treatment is reverted to original.

…rit from a virtual base and do some clean-up but leave its phase 1 behavior intact. Create a separate derived class for phase 0 (ErrorCheckerPhase0) restoring the original treatment of a few FED errors and their association to DetId-s.

…e1 is true (current default), the new phase 0 otherwise.

cmsbuild · 2019-05-07T08:39:32Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-05-07T08:45:57Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26679/9641

This PR adds an extra 32KB to repository

perrotta · 2019-05-07T08:48:25Z

please test

cmsbuild · 2019-05-07T08:48:59Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/41/console Started: 2019/05/07 10:49

cmsbuild · 2019-05-07T08:49:02Z

A new Pull Request was created by @veszpv (Viktor Veszpremi) for master.

It involves the following packages:

EventFilter/SiPixelRawToDigi

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @Martin-Grunewald, @dkotlins, @VinInn this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

perrotta

I repeat here a few comments I already made in the former version of this PR
None of them is really fundamental, but worth being taken into account anyhow before merging this PR

perrotta · 2019-05-07T08:55:58Z

EventFilter/SiPixelRawToDigi/interface/ErrorChecker.h

-class SiPixelFedCabling;
-
-class ErrorChecker {
+class ErrorChecker : public ErrorCheckerBase {

 public:
 //  typedef unsigned int Word32;


Still, you can remove this unneeded commented out code

perrotta · 2019-05-07T08:56:42Z

EventFilter/SiPixelRawToDigi/interface/ErrorCheckerBase.h

+class ErrorCheckerBase {
+
+public:
+//  typedef unsigned int Word32;


Please remove unneeded commented out code

perrotta · 2019-05-07T08:57:13Z

EventFilter/SiPixelRawToDigi/interface/ErrorCheckerPhase0.h

+class ErrorCheckerPhase0 : public ErrorCheckerBase {
+
+public:
+//  typedef unsigned int Word32;


Please remove unneeded commented out code

perrotta · 2019-05-07T08:59:16Z

EventFilter/SiPixelRawToDigi/plugins/SiPixelDigiToRaw.cc

@@ -136,7 +136,7 @@ void SiPixelDigiToRaw::produce( edm::StreamID, edm::Event& ev,

  PixelDataFormatter::BadChannels badChannels;
  edm::Handle<PixelFEDChannelCollection> pixelFEDChannelCollectionHandle;
-  if (ev.getByToken(theBadPixelFEDChannelsToken, pixelFEDChannelCollectionHandle)){
+  if (usePhase1==true && ev.getByToken(theBadPixelFEDChannelsToken, pixelFEDChannelCollectionHandle)){


usePhase1 == true can simply become usePhase1

perrotta · 2019-05-07T08:59:50Z

EventFilter/SiPixelRawToDigi/plugins/SiPixelRawToDigi.cc

@@ -252,7 +252,7 @@ void SiPixelRawToDigi::produce( edm::Event& ev,
 	  for (auto const& aPixelError : errorDetSet) {
 	    // For the time being, we extend the error handling functionality with ErrorType 25
 	    // In the future, we should sort out how the usage of tkerrorlist can be generalized
-	    if (aPixelError.getType()==25) {
+	    if (usePhase1==true && aPixelError.getType()==25) {


usePhase1 == true can simply become usePhase1

perrotta · 2019-05-07T09:06:29Z

EventFilter/SiPixelRawToDigi/src/PixelDataFormatter.cc

@@ -100,12 +100,17 @@ PixelDataFormatter::PixelDataFormatter( const SiPixelFedCabling* map, bool phase
  PXID_mask = ~(~PixelDataFormatter::Word32(0) << PXID_bits);
  ADC_mask  = ~(~PixelDataFormatter::Word32(0) << ADC_bits);

+  if (phase1==true) {


if (phase1) {

perrotta · 2019-05-07T09:17:51Z

EventFilter/SiPixelRawToDigi/src/ErrorCheckerPhase0.cc

+     break;
+   }
+   case(26) : {
+     //LogDebug("")<<"  gap word found (errorType=26)";


If these LogDebug are not neded, you can just remove them

perrotta · 2019-05-07T09:17:59Z

EventFilter/SiPixelRawToDigi/src/ErrorCheckerPhase0.cc

+     return false;
+   }
+   case(27) : {
+     //LogDebug("")<<"  dummy word found (errorType=27)";


perrotta · 2019-05-07T09:58:14Z

In order not to interfere with this PR, I opened PR #26681 for the removal of the useless ```config_.exists()''' checks in SiPixelRawToDigi.cc.
Please @veszpv fill free to comment in that PR if you have any complain with it.

cmsbuild · 2019-05-07T11:22:16Z

+1
Tested at: 37af528
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c57913/41/summary.html

cmsbuild · 2019-05-07T11:22:23Z

Comparison job queued.

perrotta · 2019-05-09T16:07:36Z

Issue #26731 have been opened to keep track of the fixes for the removal of the duplicated code that will be implemented in the next cycle after the UL release

cmsbuild · 2019-05-09T17:23:38Z

+1
Tested at: afe87d4
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c57913/130/summary.html

cmsbuild · 2019-05-09T17:23:44Z

Comparison job queued.

cmsbuild · 2019-05-09T19:54:00Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 33
DQMHistoTests: Total histograms compared: 3211964
DQMHistoTests: Total failures: 78
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3211682
DQMHistoTests: Total skipped: 204
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
Checked 137 log files, 14 edm output root files, 33 DQM output files

perrotta · 2019-05-10T08:27:33Z

EventFilter/SiPixelRawToDigi/plugins/SiPixelDigiToRaw.cc

@@ -136,7 +136,7 @@ void SiPixelDigiToRaw::produce( edm::StreamID, edm::Event& ev,

  PixelDataFormatter::BadChannels badChannels;
  edm::Handle<PixelFEDChannelCollection> pixelFEDChannelCollectionHandle;
-  if (ev.getByToken(theBadPixelFEDChannelsToken, pixelFEDChannelCollectionHandle)){
+  if (usePhase1 && ev.getByToken(theBadPixelFEDChannelsToken, pixelFEDChannelCollectionHandle)){


Ok, bad channels are there only for Phase1

perrotta · 2019-05-10T08:35:02Z

@veszpv @tsusa : in the PR description you anticipate that there is an ongoing validation for this PR. Please post here a link to the results, or just a short summary of them, so that we can conclude the review and finally sign this off for reco.

tsusa · 2019-05-10T10:34:47Z

@perrotta @slava77 The validation is still ongoing. I will post the plots as soon as they are done [ETA tomorrow]

davidlange6 · 2019-05-10T10:46:29Z

if you want to reach Slava, the correct tag is @slava77....

…

On May 10, 2019, at 12:34 PM, Tatjana Susa ***@***.***> wrote: @perrotta @Slava The validation is still ongoing. I will post the plots as soon as they are done [ETA tomorrow] — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

tsusa · 2019-05-10T10:57:27Z

Thanks @davidlange6!

fabiocos · 2019-05-11T21:09:10Z

@tsusa any news about the validation?

tsusa · 2019-05-12T13:30:03Z

@fabiocos @perrotta @slava77 The validation plots are uploaded. For Phase 1 detector shown are the occupancy maps w/ and w/o changes implemented in the PR and with BadFED simulation switched on and off. For Phase0 shown are the plots with BadFED simulation switched off as FED25 error did not exist in Phase 0 detector.

pr26679_validation.pdf

mmusich · 2019-05-13T07:26:13Z

@perrotta @fabiocos what's the plan for inclusion of this PR?
I see that 10.6.0 has been already built: as a reminder this is a mandatory fix to process phase-0 data (also included in #26721).
Does this imply that a backport will be needed?

perrotta · 2019-05-13T08:01:59Z

+1

ErrorChecker specialized for Phase0 and Phase1 separately, so that the original behaviour for Phase0 can be recovered by simply relying on the "(use)phase1" switches already present in the code
Jenkins tests show no difference: validation has been provided for both Phase0 and Phase1 configurations in Restore fed error treatment for phase0 pixel detector #26679 (comment)

cmsbuild · 2019-05-13T08:02:34Z

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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

fabiocos · 2019-05-13T08:16:23Z

+1

fabiocos · 2019-05-13T08:19:18Z

@mmusich only the externals of 10_6_0 have been pre-built. In any case at this point there is pressure to start the GEN-SIM part of production, where this PR is irrelevant

fabiocos · 2019-05-13T08:19:56Z

anyway, as this is now merged, unless problems appear in next IB it will be part of 10_6_0

mmusich · 2019-05-13T10:17:08Z

anyway, as this is now merged, unless problems appear in next IB it will be part of 10_6_0

@fabiocos thanks!

veszpv added 6 commits May 2, 2019 15:29

Disable reading bad channel collection all together for non-phase 1 d…

4b2b2c3

…etector. An empty map results in no FED25 error being packed into the RAW data.

Interpret FED error type 25 as bad channel only for phase 1. In case …

b927c41

…of phase 0, treatment is reverted to original.

Instantiate the correct error checker: the old one when variable phas…

a79e48a

…e1 is true (current default), the new phase 0 otherwise.

Apply code-check

37af528

cmsbuild added this to the CMSSW_10_6_X milestone May 7, 2019

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels May 7, 2019

veszpv mentioned this pull request May 7, 2019

Restore FED error treatment for phase0 (required for UL Rereco of 2016) #26606

Closed

cmsbuild added code-checks-approved and removed code-checks-pending labels May 7, 2019

cmsbuild added tests-started and removed tests-pending labels May 7, 2019

perrotta reviewed May 7, 2019

View reviewed changes

perrotta mentioned this pull request May 7, 2019

Get rid of useless repeated config_.exists() in SiPixelRawToDigi.cc #26681

Merged

cmsbuild added tests-approved and removed tests-started labels May 7, 2019

cmsbuild added tests-approved and removed tests-started labels May 9, 2019

cmsbuild added comparison-available and removed comparison-pending labels May 9, 2019

perrotta reviewed May 10, 2019

View reviewed changes

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels May 13, 2019

cmsbuild added orp-approved and removed orp-pending labels May 13, 2019

cmsbuild merged commit b45186e into cms-sw:master May 13, 2019

perrotta mentioned this pull request May 20, 2019

NanoAOD developments: 190520 version, part 2 for UL #26844

Merged

Restore fed error treatment for phase0 pixel detector #26679

Restore fed error treatment for phase0 pixel detector #26679

Conversation

veszpv commented May 7, 2019

cmsbuild commented May 7, 2019

cmsbuild commented May 7, 2019

perrotta commented May 7, 2019

cmsbuild commented May 7, 2019 • edited

cmsbuild commented May 7, 2019

perrotta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perrotta commented May 7, 2019

cmsbuild commented May 7, 2019

cmsbuild commented May 7, 2019

perrotta commented May 9, 2019

cmsbuild commented May 9, 2019

cmsbuild commented May 9, 2019

cmsbuild commented May 9, 2019

Choose a reason for hiding this comment

perrotta commented May 10, 2019

tsusa commented May 10, 2019 • edited

davidlange6 commented May 10, 2019 via email

tsusa commented May 10, 2019

fabiocos commented May 11, 2019

tsusa commented May 12, 2019 • edited

mmusich commented May 13, 2019

perrotta commented May 13, 2019

cmsbuild commented May 13, 2019

fabiocos commented May 13, 2019

fabiocos commented May 13, 2019

fabiocos commented May 13, 2019

mmusich commented May 13, 2019

cmsbuild commented May 7, 2019 •

edited

tsusa commented May 10, 2019 •

edited

tsusa commented May 12, 2019 •

edited