Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PixelFed25 error propagation to tracking #20151

Merged
merged 12 commits into from Oct 2, 2017

Conversation

veszpv
Copy link
Contributor

@veszpv veszpv commented Aug 14, 2017

Following changes are explained in the commits and in this presentation where validation plots also shown: https://indico.cern.ch/event/658331/#48-inactive-pixel-roc-propagat

…ink number, and the serial numbers of the first and last ROCs in the channel. Intend to list ROCs by the offline numbering scheme: an interval of N ROCs contained by [0, 15]. Also defined a collection as a DetSetVector. Overall information in the collection is sufficient to identify individual ROCs offline with DetId+ROC number.
…nnels, improved error message. 2) Cleaned up decoding TBM trailer error type 30 (allow only 'number of ROCs' and 'overflow' errors being unpacked, 3) Let error to be assigned to DetId also in the FPix (since in Phase 1 -- as opposed to Phase 0 --, one channel transmits only from a single module)
…to LogDebug in order to suppress flood of warnings generated by decoding FED error 25 for disconnected channels
…type 25 generated by disconnected FED channels. 2) Exposed information in PixelDataFormatter on bit pattern decoding of error words to raw2digi. 3) Had raw2digi populate the PixelFEDChannelCollection with type 25 error channels
…. Feature is turned on by including badPixelFEDChannelCollectionLabels in MeasurementTrackerEventProducer_cfi.py; if so, also must add pixelCablingMapLabel
@cmsbuild
Copy link
Contributor

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

It involves the following packages:

CondFormats/SiPixelObjects
DataFormats/SiPixelDetId
DataFormats/SiPixelRawData
EventFilter/SiPixelRawToDigi
RecoTracker/MeasurementDet

@perrotta, @ghellwig, @civanch, @arunhep, @cerminar, @cmsbuild, @franzoni, @mdhildreth, @slava77, @ggovi, @lpernie can you please review it and eventually sign? Thanks.
@ghellwig, @makortel, @felicepantaleo, @GiacomoSguazzoni, @rovere, @VinInn, @Martin-Grunewald, @mschrode, @dkotlins, @ebrondol, @tocheng, @gpetruc, @mmusich, @dgulhan, @seemasharmafnal this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pr-code-checks/PR-20151/91

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/jenkins-artifacts/pr-code-checks/PR-20151/91/git-diff.patch

In future, you can run scram build code-checks to apply code checks

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 14, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/22272/console Started: 2017/08/14 11:19

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2713569
  • DQMHistoTests: Total failures: 389
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2712991
  • DQMHistoTests: Total skipped: 189
  • DQMHistoTests: Total Missing objects: 0
  • Checked 107 log files, 14 edm output root files, 26 DQM output files

@slava77
Copy link
Contributor

slava77 commented Aug 14, 2017

From jenkins tests:

  • there are no changes in reco quantities
  • there are no new products saved in output files (check is done only on MC for 2016)
  • there are some differences in DQM related to pixel unpacking:

phase-0 DQM plots have typically fewer pixel errors
(left is baseline, right is with this PR)
wf136 731_addtnl_pixerrors

phase-1 DQM has changes only in /FED_xyzt in TBM message-related plots.
In wf 136.788 in most cases there are less entries but in some there are more entries
wf136 788_tbm_fed1204
wf136 788_tbm_fed1302

Are these all expected?

In the slides shown on Aug 11 there were plots showing differences in tracking related quantities.
Please suggest a run number and a range of lumis to be able to reproduce the expected behavior in reco quantities.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@slava77
Copy link
Contributor

slava77 commented Sep 29, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 29, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/23331/console Started: 2017/09/29 19:39

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2762695
  • DQMHistoTests: Total failures: 371
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2762137
  • DQMHistoTests: Total skipped: 187
  • DQMHistoTests: Total Missing objects: 0
  • Checked 107 log files, 10 edm output root files, 26 DQM output files

@slava77
Copy link
Contributor

slava77 commented Sep 29, 2017

+1

for #20151 b1e1541

  • changes are in line with the description and the follow up review; this has an effect on post-TS1 2017 data with pixel tracker included for reco quantities (unpacking of some status bits in earlier pixel data taking changed for run1 and phase-1, but that part does not affect downstream reco quantities)
  • jenkins tests pass and comparisons with baseline show no differences in reco quantities (tests do not include post-TS1 2017 data)
  • more notes on performance can be seen in the presentation in the RECO meeting linked in the PR description and also directly on this PR in PixelFed25 error propagation to tracking #20151 (comment)

I created #20703 to follow up on the code review of the POG code.

@civanch
Copy link
Contributor

civanch commented Sep 30, 2017

+1

@boudoul
Copy link
Contributor

boudoul commented Oct 1, 2017

Dear @arunhep @lpernie @ggovi , could you please consider this PR ? Thanks

@ghellwig
Copy link

ghellwig commented Oct 1, 2017

+1
@boudoul please don't blame @arunhep, @lpernie on this ;)
This PR was on my list and I was waiting for confirmation from @veszpv which I have now.

@@ -145,6 +150,15 @@ bool ErrorChecker::checkROC(bool& errorsInEvent, int fedId, const SiPixelFrameCo
}
case(30) : {
LogDebug("")<<" TBM error trailer (errorType=30)";
int StateMatch_bits = 4;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make these const and unsigned ints in a follow up pr.

if (aPixelError.getType()==25) {
assert(aPixelError.getFedId()==fedId);
const sipixelobjects::PixelFEDCabling* fed = cabling_->fed(fedId);
if (fed) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be fed != nullptr (surprising that clang-tidy did not flag this)

@davidlange6
Copy link
Contributor

merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet