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 data unpacking #35067

Merged
merged 5 commits into from
Sep 4, 2021
Merged

PPS Pixel data unpacking #35067

merged 5 commits into from
Sep 4, 2021

Conversation

fabferro
Copy link
Contributor

PR description:

This PR modifies the PPS pixel unpacking code in view of the changes for Run3.
The changes concern 13 out of 64 bits of the data word that encode the pixel coordinates: the "double column-pixel id" convention is changed into a "column-row" one, due to the use of a slightly different readout chip (PROC600 instead of PsiDIG46).
The back compatibility with Run2 data is ensured by means of era modifiers: Run3 convention becomes the default one and Run2 one is triggered by means of ctpps_201* era modifiers.
The part of the code packing the simulated data is also changed accordingly.

PR validation:

The detectors are not yet installed. They will be installed beginning of November 21. Private validation on lab data, Run2 data and full simulation was done.

if this PR is a backport please specify the original PR and why you need to backport that PR:

No backport expected.

@fabferro
Copy link
Contributor Author

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35067/24949

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • EventFilter/CTPPSRawToDigi (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @jan-kaspar this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented Aug 30, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2bbeda/18131/summary.html
COMMIT: 9832be4
CMSSW: CMSSW_12_1_X_2021-08-29-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35067/18131/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: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 39
  • DQMHistoTests: Total histograms compared: 3000352
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3000324
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

@jpata
Copy link
Contributor

jpata commented Aug 31, 2021

Could a unit test / test config be useful here?

@jpata
Copy link
Contributor

jpata commented Sep 2, 2021

Kind ping @fabferro. Just to clarify, I think this looks fine from the reco point of view, but typically we try to ask for the possibility (if it's not unreasonably complicated or useless) to have a test for new functionality, to be able to (at least in principle) run the code we sign. I think in this case a small test config on some testbeam data file could be helpful. Let me know what you think!

@fabferro
Copy link
Contributor Author

fabferro commented Sep 2, 2021

Kind ping @fabferro. Just to clarify, I think this looks fine from the reco point of view, but typically we try to ask for the possibility (if it's not unreasonably complicated or useless) to have a test for new functionality, to be able to (at least in principle) run the code we sign. I think in this case a small test config on some testbeam data file could be helpful. Let me know what you think!

It sounds reasonable. I'll ask PPS online people to provide me a(nother) data file and a temporary DAQ map.
Where can I put them for your convenience?

@jpata
Copy link
Contributor

jpata commented Sep 2, 2021

Could something be done in analogy to PR#34759, https://github.com/ChrisMisan/cmssw/blob/465b1657fb94e540dd43becf6ccb9073654b2e42/RecoPPS/Local/test/totemTiming_digiConverter_reco_cfg.py?

The test file could then perhaps be located in /eos/cms/store/group/dpg_ctpps/.

from Configuration.Eras.Modifier_ctpps_2016_cff import ctpps_2016
from Configuration.Eras.Modifier_ctpps_2017_cff import ctpps_2017
from Configuration.Eras.Modifier_ctpps_2018_cff import ctpps_2018
(ctpps_2016 | ctpps_2017 | ctpps_2018).toModify(ctppsPixelDigis, isRun3 = cms.bool(False) )
Copy link
Contributor

Choose a reason for hiding this comment

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

Slava pointed to me that the type does not need (should not) be specified here, sorry for missing it earlier.

Suggested change
(ctpps_2016 | ctpps_2017 | ctpps_2018).toModify(ctppsPixelDigis, isRun3 = cms.bool(False) )
(ctpps_2016 | ctpps_2017 | ctpps_2018).toModify(ctppsPixelDigis, isRun3 = False )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed it too when I re-read the code before submission. Slava never misses! ;-)
Thanks.
BTW, it works fine as well...

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 2, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35067/25032

  • This PR adds an extra 12KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 2, 2021

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

@fabferro
Copy link
Contributor Author

fabferro commented Sep 3, 2021

The test config just added reads lab data from /eos/cms/store/group/dpg_ctpps/comm_ctpps/PixelAlive_562_RAW.root and produces a root file with DIGIs applying a dummy DAQ map (/eos/cms/store/group/dpg_ctpps/comm_ctpps/CTPPSPixel_DAQMapping_AnalysisMask.db). The output, if limited to a few events only, can be manually compared with a binary-to-txt-dump file /eos/cms/store/group/dpg_ctpps/comm_ctpps/Run562_dumpBinary.txt.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 3, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35067/25052

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 3, 2021

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

@jpata
Copy link
Contributor

jpata commented Sep 3, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 3, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2bbeda/18281/summary.html
COMMIT: 20537f2
CMSSW: CMSSW_12_1_X_2021-09-02-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35067/18281/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: 39
  • DQMHistoTests: Total histograms compared: 3000404
  • DQMHistoTests: Total failures: 0
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3000382
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

@jpata
Copy link
Contributor

jpata commented Sep 3, 2021

+reconstruction

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 3, 2021

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

@perrotta
Copy link
Contributor

perrotta commented Sep 4, 2021

+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