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

veszpv · 2019-05-03T13:32:22Z

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-03T13:32:56Z

The code-checks are being triggered in jenkins.

slava77 · 2019-05-03T13:40:16Z

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) {


is this even needed on the raw2digi side?
Do we have cases for valid pixel error =25 in phase 0?

In phase 0, error type 25 is exotic but can happen when AFAIK the analog levels in the FED input are not interpreted correctly and the communication protocol is compromised. Such an event was actually caught by reconstruction. So this is indeed needed.

Actually this fix is mandatory for full reprocessing of 2016. Without it you will have a small but finite chunk of unprocessable data

cmsbuild · 2019-05-03T13:40:59Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26606/9549

This PR adds an extra 32KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26606/9549/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26606/9549/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2019-05-03T14:26:18Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-05-03T14:33:07Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26606/9557

This PR adds an extra 68KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26606/9557/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26606/9557/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

perrotta · 2019-05-03T15:07:54Z

@veszpv please apply code checks prescription (#26606 (comment)) so that tests can ge started

cmsbuild · 2019-05-03T15:36:58Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-05-03T15:43:43Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26606/9567

This PR adds an extra 80KB to repository

cmsbuild · 2019-05-03T15:44:20Z

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

veszpv · 2019-05-03T15:49:26Z

Note, it seems that "scram build code-format" gives result different from applying the downloaded patch.

The actual changes are quite minimal. 1) Two lines of "if" statements disable the reading/filling of the bad FED channel collection by the (un)packer code, 2) the current ErrorCheck class is made to inherit from a pure virtual base class and the old ErrorCheck from the last phase 0 release (CMSSW_9_3_14_patch1) is copied back as another derived class. Exact same code as used in the last phase 0 reco, only changed to compile in the present release.

perrotta · 2019-05-03T15:53:17Z

please test

cmsbuild · 2019-05-03T15:53:51Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/34480/console Started: 2019/05/03 17:54

cmsbuild · 2019-05-03T18:15:44Z

+1
Tested at: d840119
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26606/34480/summary.html

cmsbuild · 2019-05-03T18:15:49Z

Comparison job queued.

cmsbuild · 2019-05-03T19:51:08Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 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

The most awful thing in this PR is the line splitting as it was automatically implemented by the code checks: it makes the code much less easily readable in several parts, besides complicating quite a lot the review by adding tons of unneeded (at best) or worsening changes. In the following review I tried to avoid commenting about those splittings. but it would be nice if they could be reverted here, and their implementation not enforced any more in the clang checks. or at least somehow tuned.

perrotta · 2019-05-06T15:09:18Z

EventFilter/SiPixelRawToDigi/interface/ErrorCheckerBase.h

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


Please remove commented out code when not needed

perrotta · 2019-05-06T15:10:36Z

EventFilter/SiPixelRawToDigi/interface/ErrorChecker.h

 public:
-//  typedef unsigned int Word32;
-//  typedef long long Word64;
+  //  typedef unsigned int Word32;


Please remove commented out code when not needed

perrotta · 2019-05-06T15:10:45Z

EventFilter/SiPixelRawToDigi/interface/ErrorCheckerPhase0.h

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


Please remove commented out code when not needed

perrotta · 2019-05-06T15:17:29Z

EventFilter/SiPixelRawToDigi/interface/PixelDataFormatter.h

+
+  // For the 32bit data format (moved from *.cc namespace, keep uppercase for
+  // compatibility) Add special layer 1 roc for phase1
+  int ADC_shift, PXID_shift, DCOL_shift, ROC_shift, LINK_shift, ROW_shift,


I really don't like this way of splitting the lines of code.
I understand that this is not you, but rather clang... Let it as such, then.

perrotta · 2019-05-06T15:20:29Z

EventFilter/SiPixelRawToDigi/interface/PixelUnpackingRegions.h

- *        tolerances could be given to each input collection.
- * Output: FED ids and module detIds that need to be unpacked
+ *        defining the directions of unpacking regions; separate deltaPhi and
+ * maxZ tolerances could be given to each input collection. Output: FED ids and


This should be better fixed and reported to the previous indentation and line splitting, for better readability.
Let profit that checks have been disabled till the closure of pre6 :-)

perrotta · 2019-05-06T15:59:22Z

EventFilter/SiPixelRawToDigi/src/PixelDataFormatter.cc

  LogDebug("PixelDataFormatter")
-// <<" detId: " << detId 
-  <<print(digi);
+      // <<" detId: " << detId


remove unneeded commented out line

perrotta · 2019-05-06T15:59:38Z

EventFilter/SiPixelRawToDigi/src/PixelDataFormatter.cc

+  if (fedId < 0)
+    return fedId;
+
+  // if(DANEK) cout<<" digi2raw "<<detId<<" "<<digi.column()<<" "<<digi.row()<<"


Please remove these commented out lines if not needed any more

perrotta · 2019-05-06T16:00:03Z

EventFilter/SiPixelRawToDigi/src/PixelDataFormatter.cc

  LogDebug("PixelDataFormatter")
-// <<" detId: " << detId 
-  <<print(digi);
+      // <<" detId: " << detId


perrotta · 2019-05-06T16:00:13Z

EventFilter/SiPixelRawToDigi/src/PixelDataFormatter.cc


-  //if(DANEK) cout<<" layer 1: digi2raw "<<detId<<" "<<digi.column()<<" "<<digi.row()<<" "<<digi.adc()<<" "
-  //  <<cabling.link<<" "<<cabling.roc<<" "<<cabling.dcol<<" "<<cabling.pxid<<" "
+  // if(DANEK) cout<<" layer 1: digi2raw "<<detId<<" "<<digi.column()<<"


Please remove these commented out lines if not needed any more

perrotta · 2019-05-06T16:01:31Z

EventFilter/SiPixelRawToDigi/src/PixelUnpackingRegions.cc

@@ -58,18 +61,16 @@ void PixelUnpackingRegions::run(const edm::Event& e, const edm::EventSetup& es)
  edm::Handle<reco::BeamSpot> beamSpot;
  e.getByToken(tBeamSpot, beamSpot);
  beamSpot_ = beamSpot->position();
-  //beamSpot_ = math::XYZPoint(0.,0.,0.);
+  // beamSpot_ = math::XYZPoint(0.,0.,0.);


veszpv · 2019-05-06T17:07:53Z

I am not sure how to undo the changes by code check. If you want, I can try to run
git push origin +HEAD
after I removed the commits from my local repo. Would that help? Or I can submit another PR. You are commenting on code that has nothing to do with this PR. Also, I do not think it is feasible to review the entire SiPixelRawToDigi package now.

perrotta · 2019-05-06T21:34:33Z

I am not sure how to undo the changes by code check. If you want, I can try to run
git push origin +HEAD
after I removed the commits from my local repo. Would that help? Or I can submit another PR. You are commenting on code that has nothing to do with this PR. Also, I do not think it is feasible to review the entire SiPixelRawToDigi package now.

Please don't mind if I don't like the clang prescriptions of the code checks: you can just concentrate on my comments for now.

Otherwise, you could submit another PR which only contains your original commits, profiting of the fact that code checks have been now momentarily disabled in order to speed up integrations for pre5.

veszpv · 2019-05-07T08:39:55Z

Resubmitted this PR as PR26679 without the format checks for easier review.

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 3, 2019

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

slava77 reviewed May 3, 2019

View reviewed changes

cmsbuild added code-checks-rejected and removed code-checks-pending labels May 3, 2019

Apply cosmetics

90d35ed

cmsbuild added code-checks-pending and removed code-checks-rejected labels May 3, 2019

cmsbuild added code-checks-rejected and removed code-checks-pending labels May 3, 2019

Apply cosmetics Round 2

d840119

cmsbuild added code-checks-pending and removed code-checks-rejected labels May 3, 2019

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

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

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

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

tsusa mentioned this pull request May 5, 2019

Output BadFED Channel only for Phase 1 #26636

Merged

perrotta reviewed May 6, 2019

View reviewed changes

perrotta mentioned this pull request May 7, 2019

Add more calorimeter energy fractions to PackedCandidates #26506

Merged

veszpv mentioned this pull request May 7, 2019

Restore fed error treatment for phase0 pixel detector #26679

Merged

veszpv closed this May 7, 2019

perrotta mentioned this pull request May 7, 2019

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

veszpv commented May 3, 2019

cmsbuild commented May 3, 2019

slava77 May 3, 2019

veszpv May 3, 2019

mmusich May 3, 2019

cmsbuild commented May 3, 2019

cmsbuild commented May 3, 2019

cmsbuild commented May 3, 2019

perrotta commented May 3, 2019

cmsbuild commented May 3, 2019

cmsbuild commented May 3, 2019

cmsbuild commented May 3, 2019

veszpv commented May 3, 2019

perrotta commented May 3, 2019

cmsbuild commented May 3, 2019 •

edited

Loading

cmsbuild commented May 3, 2019

cmsbuild commented May 3, 2019

cmsbuild commented May 3, 2019

perrotta left a comment

perrotta May 6, 2019

perrotta May 6, 2019

perrotta May 6, 2019

perrotta May 6, 2019

perrotta May 6, 2019

perrotta May 6, 2019

perrotta May 6, 2019

perrotta May 6, 2019

perrotta May 6, 2019

perrotta May 6, 2019

veszpv commented May 6, 2019

perrotta commented May 6, 2019

veszpv commented May 7, 2019

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

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

Conversation

veszpv commented May 3, 2019

PR description:

PR validation:

cmsbuild commented May 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented May 3, 2019

cmsbuild commented May 3, 2019

cmsbuild commented May 3, 2019

perrotta commented May 3, 2019

cmsbuild commented May 3, 2019

cmsbuild commented May 3, 2019

cmsbuild commented May 3, 2019

veszpv commented May 3, 2019

perrotta commented May 3, 2019

cmsbuild commented May 3, 2019 • edited Loading

cmsbuild commented May 3, 2019

cmsbuild commented May 3, 2019

cmsbuild commented May 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veszpv commented May 6, 2019

perrotta commented May 6, 2019

veszpv commented May 7, 2019

cmsbuild commented May 3, 2019 •

edited

Loading