PPS global alignment - reformat #36257

MatiXOfficial · 2021-11-26T14:50:35Z

PR description:

This is the last planned PR for PPS global alignment. It does not change the logic of the code nor anything in the PCL workflow configuration and the matrix test. The main purpose is reformatting the code, updating the README files and adding one more parameter to PPSAlignmentHarvester - reasonability range - which ensures that the harvester will not produce NaN or extremely big alignment corrections in the output.

Apart from the CMSSW changes, a new TWiki page was created, and some part of the documentation that previously had been in the README files was moved there.

For further information about including global alignment in CMSSW check out two previous PRs: #35631 and #35874.

PR validation:

The 1042 relval runs correctly.
The test from CalibPPS/AlignmentGloba/test reproduced the results.
Another tests based on Run 2 data also reproduced results.

- Replaced some raw pointers with the smart ones in PPSAlignmentConfigurationESSource. - Simplified pair iteration in PPSAlignmentHarvester, xAlignment.

If the corrections created by the harvester are not compatible with these ranges, they are replaced with 0.

- Updated the names of some variables. - Improved formatting in a few places. - Modified some comments and added new ones. - Removed unnecessary imports. - Added const where applicable. - Replaced some function parameters with class/struct members.

cmsbuild · 2021-11-26T14:56:41Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36257/26916

This PR adds an extra 52KB to repository

cmsbuild · 2021-11-26T14:57:07Z

A new Pull Request was created by @MatiXOfficial (Mateusz Kocot) for master.

It involves the following packages:

CalibPPS/AlignmentGlobal (alca)
CalibPPS/ESProducers (alca)
CondFormats/PPSObjects (alca)

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

malbouis · 2021-11-26T15:15:25Z

test parameters:

workflows = 1042

malbouis · 2021-11-26T15:16:02Z

@cmsbuild , please test

cmsbuild · 2021-11-26T18:19:19Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ce6a23/20788/summary.html
COMMIT: 88d93fd
CMSSW: CMSSW_12_2_X_2021-11-26-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36257/20788/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-ce6a23/1042.0_RunExpressPhy2017F+RunExpressPhy2017F+TIER0EXPPPSCALALIG+ALCASPLITPPSALIG+ALCAHARVDPPSALIG

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 3247873
DQMHistoTests: Total failures: 5
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3247845
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 41 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

tvami · 2021-11-26T23:38:11Z

CalibPPS/ESProducers/plugins/PPSAlignmentConfigESSource.cc

- *  CalibPPS/ESProducers/plugins/PPSAlignmentConfigESSource.cc
- * 
- *  Description: Constructs PPSAlignmentConfig instance
+ *  This module is obsolete! Use PPSAlignmentConfigurationESSource instead.


Should there be a real exception thrown if somebody wants to use this? I'm not sure a single comment in the code is sufficient to stop people to use it haha

I just wanted to make sure no one is confused when they see two very similar classes in one directory, but throwing an exception is a good idea too. I'll add it.

if the module is obsolete, what's the reason to keep it in the code-base?
Are there still clients of it?

PPSAlignmentConfiguration has fully replaced PPSAlignmentConfig, so nobody will use its ESSource in the future. To be honest, I wanted to delete all the PPSAlignmentConfig auxiliary classes in one of the previous PRs (#35174 (comment)), but eventually we decided to keep them. Do you suggest to delete them now?

the less code is in the stack, the less maintenance it will require. If the functionality is fully replaced by other modules, deleting the obsolete files sounds like a good option (and will also automatically enforce that nobody uses them by mistake).

Ok then, I'll delete them. Am I right though that I can't delete the PPSAlignmentConfig class? It has a few payloads in the DB.

Am I right though that I can't delete the PPSAlignmentConfig class? It has a few payloads in the DB.

provided they are not included in any autoCond GlobalTag you technically could delete the C++ class as well, though if I am not mistaken AlCa/DB discourages that practice @cms-sw/alca-l2 can comment further.

Ok, thanks! I will leave the class for now.

CondFormats/PPSObjects/src/PPSAlignmentConfig.cc

CalibPPS/AlignmentGlobal/plugins/PPSAlignmentHarvester.cc

cmsbuild · 2021-12-06T19:28:42Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ce6a23/21024/summary.html
COMMIT: 21fb68d
CMSSW: CMSSW_12_2_X_2021-12-06-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36257/21024/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-ce6a23/1042.0_RunExpressPhy2017F+RunExpressPhy2017F+TIER0EXPPPSCALALIG+ALCASPLITPPSALIG+ALCAHARVDPPSALIG

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 3250608
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3250580
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

tvami · 2021-12-06T19:45:39Z

+alca

code change in line w PR description and code review
Jenkins test pass

cmsbuild · 2021-12-06T19:46:03Z

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)

tvami · 2021-12-06T19:47:01Z

CondCore/Utilities/src/CondDBFetch.cc

@@ -56,7 +56,6 @@ namespace cond {
      FETCH_PAYLOAD_CASE(CTPPSPixelAnalysisMask)
      FETCH_PAYLOAD_CASE(CTPPSPixelGainCalibrations)
      FETCH_PAYLOAD_CASE(CTPPSRPAlignmentCorrectionsData)


@ggovi is this deletion fine?

@tvami It is only relevant for conddb_import, which is an obsolete command anyhow... Therefore yes, it is fine.

tvami · 2021-12-07T13:10:03Z

+db

cmsbuild · 2021-12-07T13:10:26Z

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)

qliphy · 2021-12-08T01:03:35Z

+1

MatiXOfficial added 9 commits November 2, 2021 20:49

Small reformat

a20f497

- Replaced some raw pointers with the smart ones in PPSAlignmentConfigurationESSource. - Simplified pair iteration in PPSAlignmentHarvester, xAlignment.

Moved fitProfile to utils.h.

03b1ef1

Added reasonability ranges to the harvester.

b11b4ab

If the corrections created by the harvester are not compatible with these ranges, they are replaced with 0.

Replaced folder variables with the ones with more reasonable names.

e217f09

AlignmentGlobal package reformat

0b6542a

- Updated the names of some variables. - Improved formatting in a few places. - Modified some comments and added new ones. - Removed unnecessary imports. - Added const where applicable. - Replaced some function parameters with class/struct members.

Moved a small part of the code to a new SlicePlots method.

34b2e20

Updated the READMEs in AlignmentGlobal.

e4c27fa

Changed the tag in the AlignmentGlobal test config.

9252c9b

Added comments to the PPSAlignmentConfig classes to mark them obsolete.

88d93fd

cmsbuild added this to the CMSSW_12_2_X milestone Nov 26, 2021

cmsbuild added alca-pending code-checks-pending orp-pending pending-signatures tests-pending labels Nov 26, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 26, 2021

cmsbuild added tests-started and removed tests-pending labels Nov 26, 2021

cmsbuild added tests-approved and removed tests-started labels Nov 26, 2021

tvami reviewed Nov 26, 2021

View reviewed changes

CondFormats/PPSObjects/src/PPSAlignmentConfig.cc Show resolved Hide resolved

tvami reviewed Nov 26, 2021

View reviewed changes

CalibPPS/AlignmentGlobal/plugins/PPSAlignmentHarvester.cc Outdated Show resolved Hide resolved

tvami reviewed Nov 26, 2021

View reviewed changes

CalibPPS/AlignmentGlobal/plugins/PPSAlignmentHarvester.cc Outdated Show resolved Hide resolved

cmsbuild added tests-started and removed tests-pending labels Dec 6, 2021

cmsbuild added tests-approved and removed tests-started labels Dec 6, 2021

cmsbuild added alca-approved db-approved fully-signed and removed alca-pending db-pending pending-signatures labels Dec 6, 2021

cmsbuild added db-pending pending-signatures and removed fully-signed db-approved labels Dec 6, 2021

tvami reviewed Dec 6, 2021

View reviewed changes

smuzaffar modified the milestones: CMSSW_12_2_X, CMSSW_12_3_X Dec 6, 2021

cmsbuild added db-approved fully-signed and removed db-pending pending-signatures labels Dec 7, 2021

cmsbuild added orp-approved and removed orp-pending labels Dec 8, 2021

cmsbuild merged commit 25cfff6 into cms-sw:master Dec 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PPS global alignment - reformat #36257

PPS global alignment - reformat #36257

MatiXOfficial commented Nov 26, 2021

cmsbuild commented Nov 26, 2021

cmsbuild commented Nov 26, 2021

malbouis commented Nov 26, 2021

malbouis commented Nov 26, 2021

cmsbuild commented Nov 26, 2021

tvami Nov 26, 2021

MatiXOfficial Nov 27, 2021

mmusich Nov 27, 2021

MatiXOfficial Nov 27, 2021

mmusich Nov 27, 2021

MatiXOfficial Nov 27, 2021

mmusich Nov 27, 2021

MatiXOfficial Nov 27, 2021

cmsbuild commented Dec 6, 2021

tvami commented Dec 6, 2021 •

edited

Loading

cmsbuild commented Dec 6, 2021

tvami Dec 6, 2021

ggovi Dec 7, 2021

tvami commented Dec 7, 2021

cmsbuild commented Dec 7, 2021

qliphy commented Dec 8, 2021

PPS global alignment - reformat #36257

PPS global alignment - reformat #36257

Conversation

MatiXOfficial commented Nov 26, 2021

PR description:

PR validation:

cmsbuild commented Nov 26, 2021

cmsbuild commented Nov 26, 2021

malbouis commented Nov 26, 2021

malbouis commented Nov 26, 2021

cmsbuild commented Nov 26, 2021

Comparison Summary

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

cmsbuild commented Dec 6, 2021

Comparison Summary

tvami commented Dec 6, 2021 • edited Loading

cmsbuild commented Dec 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvami commented Dec 7, 2021

cmsbuild commented Dec 7, 2021

qliphy commented Dec 8, 2021

tvami commented Dec 6, 2021 •

edited

Loading