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

New PCL Alignment Threshold object and record for HG PCL Alignment #38195

Merged
merged 1 commit into from
Jun 4, 2022

Conversation

dmeuser
Copy link
Contributor

@dmeuser dmeuser commented Jun 2, 2022

PR description:

This PR is the first step towards using a higher granularity (HG) for the pixel alignment in the PCL. A new threshold object is introduced, which allows to use additional cuts/vetoes on top of the ones already used in the low granularity (LG) alignment.

In detail the following changes/adaption have been made:

  • New class AlignPCLThresholdsHG, which inherits from AlignPCLThresholds, and adds containers for floats, ints and strings with variable sizes to allow for different types of new thresholds in the future. So far, only methods for a fraction cut are implemented.
  • New record AlignPCLThresholdsHGRcd, which is based on the new class AlignPCLThresholdsHG
  • Templated AlignPCLThresholdsWriter and AlignPCLThresholdsReader to handle AlignPCLThresholdsHG and AlignPCLThresholds
  • Adapt the config of AlignPCLThresholdsReader to handle both payload types

PR validation:

The PR can be tested with the old payload type based on AlignPCLThresholdsHG and the new payload type based on AlignPCLThresholdsHG:

  • Testing the new payload type: cmsRun AlignPCLThresholdsWriter_cfg.py && cmsRun AlignPCLThresholdsWriter_cfg.py
  • Testing the old payload type: Remove fractionCut entries in Thresholds_cff.py and run cmsRun AlignPCLThresholdsWriter_cfg.py && cmsRun AlignPCLThresholdsWriter_cfg.py readLGpayload=True

Upcoming PRs:

This PR will be followed by two more PRs:

  • Changing to the new object in the consumer code in Alingment package
  • Introducing the changes needed to run and store the HG alignment

cc:
@mmusich, @connorpa, @antoniovagnerini, @consuegs

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 2, 2022

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38195/30347

  • This PR adds an extra 64KB to repository

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 2, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38195/30348

  • This PR adds an extra 64KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 2, 2022

A new Pull Request was created by @dmeuser for master.

It involves the following packages:

  • CondCore/PCLConfigPlugins (db)
  • CondCore/Utilities (db)
  • CondFormats/DataRecord (db, alca)
  • CondFormats/PCLConfig (db, alca)

@malbouis, @yuanchao, @cmsbuild, @ggovi, @francescobrivio, @tvami can you please review it and eventually sign? Thanks.
@mmusich, @tocheng, @seemasharmafnal 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

@mmusich
Copy link
Contributor

mmusich commented Jun 2, 2022

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 2, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-502799/25197/summary.html
COMMIT: 000a89d
CMSSW: CMSSW_12_5_X_2022-06-02-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38195/25197/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: 50
  • DQMHistoTests: Total histograms compared: 3649923
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3649893
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 3, 2022

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38195/30378

  • This PR adds an extra 104KB to repository

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 3, 2022

Pull request #38195 was updated. @malbouis, @yuanchao, @cmsbuild, @ggovi, @francescobrivio, @tvami can you please check and sign again.

@tvami
Copy link
Contributor

tvami commented Jun 3, 2022

@cmsbuild , please test

@tvami
Copy link
Contributor

tvami commented Jun 3, 2022

+1

  • code change in line with PR description
  • test pass
  • last change is just a squash, so signing early

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 3, 2022

This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. 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)

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 3, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-502799/25259/summary.html
COMMIT: be1d3ea
CMSSW: CMSSW_12_5_X_2022-06-03-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38195/25259/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: 50
  • DQMHistoTests: Total histograms compared: 3651513
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3651483
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@qliphy
Copy link
Contributor

qliphy commented Jun 4, 2022

+1

@cmsbuild cmsbuild merged commit c1235c5 into cms-sw:master Jun 4, 2022
@tvami
Copy link
Contributor

tvami commented Jun 4, 2022

@dmeuser @mmusich the plan for backports is to wait for the follow-up PRs to this one and have one big backport PR, right?

@mmusich
Copy link
Contributor

mmusich commented Jun 4, 2022

for the follow-up PRs to this one and have one big backport PR, right?

That was the plan indeed.

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.

6 participants