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

PPS pixel bug fix in error filling #40660

Merged
merged 2 commits into from
Feb 3, 2023

Conversation

fabferro
Copy link
Contributor

@fabferro fabferro commented Feb 1, 2023

PR description:

This PR is a bug fix in filling the persistent information about the errors coming from PPS Pixels during the data unpacking.
Data unpacking is not affected, only the way errors are decoded.
This PR is needed by a forthcoming PR to update the PPS Pixel DQM in order to display errors.

PR validation:

Validation has been done reprocessing Run-3 data (AlCaPPS data of run 355921, ~1M events) and running WF 136.892 of RunThematrix.
The tests reported no changes in unpacked data, a part from the desired new error encoding.

No backport needed.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40660/33989

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2023

A new Pull Request was created by @fabferro (Fabrizio Ferro) for master.

It involves the following packages:

  • CondFormats/PPSObjects (alca)
  • EventFilter/CTPPSRawToDigi (reconstruction)

@malbouis, @yuanchao, @ChrisMisan, @clacaputo, @cmsbuild, @saumyaphor4252, @tvami, @mandrenguyen, @francescobrivio can you please review it and eventually sign? Thanks.
@forthommel, @tocheng, @Martin-Grunewald, @missirol, @grzanka, @mmusich, @seemasharmafnal this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@perrotta
Copy link
Contributor

perrotta commented Feb 1, 2023

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2023

-1

Failed Tests: RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0fed64/30320/summary.html
COMMIT: f5b2421
CMSSW: CMSSW_13_0_X_2023-01-31-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40660/30320/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

The relvals timed out after 4 hours.

Comparison Summary

Summary:

  • You potentially added 1 lines to the logs
  • Reco comparison results: 6 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3555495
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3555467
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 211 log files, 162 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@fabferro
Copy link
Contributor Author

fabferro commented Feb 1, 2023

It seems relvals are failing because of unreachable input files.

@francescobrivio
Copy link
Contributor

francescobrivio commented Feb 1, 2023

@cmsbuild please test

@francescobrivio
Copy link
Contributor

It seems relvals are failing because of unreachable input files.

Relval timeouts happen from time to time. Let's try again!

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2023

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0fed64/30330/summary.html
COMMIT: f5b2421
CMSSW: CMSSW_13_0_X_2023-01-31-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40660/30330/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 3 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3555495
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3555470
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 211 log files, 162 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@francescobrivio
Copy link
Contributor

type bugfix

@francescobrivio
Copy link
Contributor

+alca

  • from the alca point of view it's just the addition of a simple method
  • tests are clean

@clacaputo
Copy link
Contributor

+reconstruction

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 3, 2023

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. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

auto rawId = rocp.rawId();

detDigis = &digis.find_or_insert(rawId);
if ((*detDigis).empty())
(*detDigis).data.reserve(32); // avoid the first relocations
}

if (skipROC || rocp.rawId() == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

@fabferro , just for checking: is it correct that skipROC is defined outside the loop, i.e. it remains set at the value taken during the previous iterations in the loop over word's?

Copy link
Contributor Author

@fabferro fabferro Feb 3, 2023

Choose a reason for hiding this comment

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

Thanks Andrea for the question. The use of this flag is very delicate. The way it's assumed to work is that if it happens to be set true (i.e. a roc chip behaving badly) all the words from that roc are skipped. The flag can change inside the loop when the roc number changes (see lines 156-162) and the new roc is checked.

@perrotta
Copy link
Contributor

perrotta commented Feb 3, 2023

+1

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.

5 participants