Migrate to esConsumes muon alignment #35719

hyunyong · 2021-10-18T19:13:06Z

PR description:

esConsumes migration for

Alignment/MuonAlignment/src/MuonAlignmentInputXML.cc
Alignment/MuonAlignment/src/MuonAlignmentInputMethod.cc
Alignment/MuonAlignment/src/MuonAlignmentInputSurveyDB.cc
Alignment/MuonAlignment/src/MuonAlignmentOutputXML.cc
Alignment/MuonAlignment/plugins/MuonGeometrySanityCheck.cc
Alignment/MuonAlignmentAlgorithms/plugins/CSCOverlapsTrackPreparation.cc
Alignment/MuonAlignmentAlgorithms/src/SegmentToTrackAssociator.cc

PR validation:

Alignment/MuonAlignment/test/test_MuonGeometryDBConverter.sh is working correctly.

tvami

To be consistent with the rest of the code, and also the coding rules

tvami · 2021-10-18T19:18:24Z

Alignment/MuonAlignment/interface/MuonAlignmentInputSurveyDB.h

@@ -30,8 +30,13 @@

 class MuonAlignmentInputSurveyDB : public MuonAlignmentInputMethod {
 public:
-  MuonAlignmentInputSurveyDB();
-  MuonAlignmentInputSurveyDB(std::string dtLabel, std::string cscLabel, std::string idealLabel);
+  MuonAlignmentInputSurveyDB(const DTGeometry* DTGeometry,


Suggested change

MuonAlignmentInputSurveyDB(const DTGeometry* DTGeometry,

MuonAlignmentInputSurveyDB(const DTGeometry* dtGeometry,

Yes, I fixed it.

tvami · 2021-10-18T19:18:41Z

Alignment/MuonAlignment/interface/MuonAlignmentInputSurveyDB.h

-  MuonAlignmentInputSurveyDB();
-  MuonAlignmentInputSurveyDB(std::string dtLabel, std::string cscLabel, std::string idealLabel);
+  MuonAlignmentInputSurveyDB(const DTGeometry* DTGeometry,
+                             const CSCGeometry* CSCGeometry,


Suggested change

const CSCGeometry* CSCGeometry,

const CSCGeometry* cscGeometry,

Yes, I fixed it.

tvami · 2021-10-18T19:18:51Z

Alignment/MuonAlignment/interface/MuonAlignmentInputSurveyDB.h

-  MuonAlignmentInputSurveyDB(std::string dtLabel, std::string cscLabel, std::string idealLabel);
+  MuonAlignmentInputSurveyDB(const DTGeometry* DTGeometry,
+                             const CSCGeometry* CSCGeometry,
+                             const GEMGeometry* GEMGeometry,


Suggested change

const GEMGeometry* GEMGeometry,

const GEMGeometry* gemGeometry,

Yes, I fixed it.

cmsbuild · 2021-10-18T19:20:08Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35719/26037

This PR adds an extra 68KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File Alignment/MuonAlignment/src/MuonAlignment.cc modified in PR(s): Migrate to the new ref. based writeOneIOV() in Alignment package #35711
- File Alignment/MuonAlignmentAlgorithms/test/StandAloneTest.cc modified in PR(s): Added esConsumes calls to more Muon related code #35697

cmsbuild · 2021-10-18T19:20:31Z

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

It involves the following packages:

Alignment/MuonAlignment (alca)
Alignment/MuonAlignmentAlgorithms (alca)
Alignment/SurveyAnalysis (alca)

@cmsbuild, @malbouis, @tvami, @yuanchao, @francescobrivio can you please review it and eventually sign? Thanks.
@pakhotin, @adewit, @abbiendi, @jhgoh, @tocheng, @tlampen, @mmusich, @trocino 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

Dr15Jones · 2021-10-18T19:23:06Z

Alignment/MuonAlignment/plugins/MuonGeometryArrange.cc

+    edm::ESHandle<DTGeometry> dtGeometry1;
+    edm::ESHandle<CSCGeometry> cscGeometry1;
+    edm::ESHandle<GEMGeometry> gemGeometry1;
+    dtGeometry1 = iSetup.getHandle(dtGeomToken1_);


You still need to call esConsumes with the appropriate labels in the module's constructor in order to initialize the tokens so they know what you want to get.

Yes, I fixed it.

tvami · 2021-10-18T19:26:37Z

Alignment/MuonAlignment/plugins/MuonGeometryArrange.h

+  const edm::ESGetToken<DTGeometry, MuonGeometryRecord> dtGeomToken1_;
+  const edm::ESGetToken<CSCGeometry, MuonGeometryRecord> cscGeomToken1_;
+  const edm::ESGetToken<GEMGeometry, MuonGeometryRecord> gemGeomToken1_;
+
+  const edm::ESGetToken<DTGeometry, MuonGeometryRecord> dtGeomToken2_;
+  const edm::ESGetToken<CSCGeometry, MuonGeometryRecord> cscGeomToken2_;
+  const edm::ESGetToken<GEMGeometry, MuonGeometryRecord> gemGeomToken2_;
+
+  const edm::ESGetToken<DTGeometry, MuonGeometryRecord> dtGeomToken3_;
+  const edm::ESGetToken<CSCGeometry, MuonGeometryRecord> cscGeomToken3_;
+  const edm::ESGetToken<GEMGeometry, MuonGeometryRecord> gemGeomToken3_;
+
+  const edm::ESGetToken<DTGeometry, MuonGeometryRecord> dtGeomIdealToken_;
+  const edm::ESGetToken<CSCGeometry, MuonGeometryRecord> cscGeomIdealToken_;
+  const edm::ESGetToken<GEMGeometry, MuonGeometryRecord> gemGeomIdealToken_;


I dont understand why you need these 3+1 times, these are the inputs to 3 different methods but since the input doesnt change (especially since they are const) why not use the same thing?

Ahh based on https://github.com/cms-sw/cmssw/pull/35719/files#r731246134 I understand that these were supposed to have different labels, fine then I understand. But then can they be names so they better describe the content?

MuonAlignmentInputXML() needs one ideal geometry set for the aligner and another ideal geometry set for comparison.
This code has three MuonAlignmentInputXML(), so three tokens (1,2, and 3) for the aligner and idealToken for the comparison.

Right, after reading #35719 (comment) I understood it

tvami · 2021-10-18T22:04:43Z

@cmsbuild , please test

cmsbuild · 2021-10-18T22:08:20Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35719/26046

This PR adds an extra 72KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File Alignment/MuonAlignment/src/MuonAlignment.cc modified in PR(s): Migrate to the new ref. based writeOneIOV() in Alignment package #35711
- File Alignment/MuonAlignmentAlgorithms/test/StandAloneTest.cc modified in PR(s): Added esConsumes calls to more Muon related code #35697

cmsbuild · 2021-10-18T22:08:46Z

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

makortel · 2021-10-19T01:03:47Z

Alignment/MuonAlignment/plugins/MuonGeometryArrange.cc

+    gemGeometryIdeal = iSetup.getHandle(gemGeomIdealToken_);
+
+    MuonAlignmentInputXML inputMethod1(_inputXMLCurrent,
+                                       &*dtGeometry1,


Just to note that the intermediate ESHandles could be avoided with

Suggested change

&*dtGeometry1,

&iSetup.getData(dtGeomToken1_),

Yes, I removed the intermediate ESHandle usage.

cmsbuild · 2021-10-19T02:02:12Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ae46bb/19725/summary.html
COMMIT: 81220fe
CMSSW: CMSSW_12_1_X_2021-10-18-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35719/19725/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 2 inherits from legacy modules warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ae46bb/19725/llvm-analysis/legacy-mod-sa.txt for details.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 10 differences found in the comparisons
DQMHistoTests: Total files compared: 40
DQMHistoTests: Total histograms compared: 2751113
DQMHistoTests: Total failures: 11
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2751079
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 39 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 170 log files, 37 edm output root files, 40 DQM output files
TriggerResults: no differences found

tvami · 2021-10-19T06:51:37Z

Hi @hyunyong besides Matti's comment it would be nice to deal with the SA errors too, reported here
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ae46bb/19725/llvm-analysis/legacy-mod-sa.txt

cmsbuild · 2021-10-20T14:43:24Z

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

cmsbuild · 2021-10-20T14:51:31Z

-1

Failed Tests: Build HeaderConsistency ClangBuild
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ae46bb/19771/summary.html
COMMIT: 7e93281
CMSSW: CMSSW_12_1_X_2021-10-19-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35719/19771/install.sh to create a dev area with all the needed externals and cmssw changes.

Build

I found compilation warning when building: See details on the summary page.

Clang Build

I found compilation warning while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 32 COMPILER='llvm compile'

See details on the summary page.

cmsbuild · 2021-10-20T15:06:24Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35719/26093

This PR adds an extra 24KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File Alignment/MuonAlignment/src/MuonAlignment.cc modified in PR(s): Migrate to the new ref. based writeOneIOV() in Alignment package #35711

cmsbuild · 2021-10-20T15:06:46Z

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

tvami · 2021-10-20T15:18:33Z

@cmsbuild , please test

cmsbuild · 2021-10-20T18:49:30Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ae46bb/19774/summary.html
COMMIT: 906a1fc
CMSSW: CMSSW_12_1_X_2021-10-19-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35719/19774/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: 40
DQMHistoTests: Total histograms compared: 2751113
DQMHistoTests: Total failures: 11
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2751079
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 39 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 170 log files, 37 edm output root files, 40 DQM output files
TriggerResults: no differences found

tvami · 2021-10-20T18:55:57Z

+alca

test pass, diffs only in MsgLogger
code is in line with the description, follow-up to the review was carried out

cmsbuild · 2021-10-20T18:56:15Z

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-10-21T00:43:09Z

+1

cmsbuild added this to the CMSSW_12_1_X milestone Oct 18, 2021

cmsbuild added alca-pending code-checks-pending orp-pending pending-signatures tests-pending labels Oct 18, 2021

hyunyong mentioned this pull request Oct 18, 2021

[Draft] esConsumes migration #35659

Closed

tvami reviewed Oct 18, 2021

View reviewed changes

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 18, 2021

Dr15Jones reviewed Oct 18, 2021

View reviewed changes

tvami reviewed Oct 18, 2021

View reviewed changes

cmsbuild added code-checks-pending and removed code-checks-approved labels Oct 18, 2021

cmsbuild added tests-started and removed tests-pending labels Oct 18, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 18, 2021

makortel reviewed Oct 19, 2021

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Oct 19, 2021

cmsbuild removed tests-approved code-checks-approved labels Oct 19, 2021

cmsbuild added tests-rejected and removed tests-started labels Oct 20, 2021

remove BuildFile.xml

906a1fc

cmsbuild added code-checks-pending tests-pending and removed tests-rejected code-checks-approved labels Oct 20, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 20, 2021

cmsbuild added tests-started and removed tests-pending labels Oct 20, 2021

cmsbuild added tests-approved and removed tests-started labels Oct 20, 2021

cmsbuild added alca-approved fully-signed and removed alca-pending pending-signatures labels Oct 20, 2021

cmsbuild added orp-approved and removed orp-pending labels Oct 21, 2021

cmsbuild merged commit 8178c08 into cms-sw:master Oct 21, 2021

tvami mentioned this pull request Oct 25, 2021

Deprecated ES get functions in Alignment/MuonAlignment cms-AlCaDB/AlCaTools#39

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to esConsumes muon alignment #35719

Migrate to esConsumes muon alignment #35719

hyunyong commented Oct 18, 2021 •

edited

Loading

tvami left a comment

tvami Oct 18, 2021

hyunyong Oct 18, 2021

tvami Oct 18, 2021

hyunyong Oct 18, 2021

tvami Oct 18, 2021

hyunyong Oct 18, 2021

cmsbuild commented Oct 18, 2021

cmsbuild commented Oct 18, 2021

Dr15Jones Oct 18, 2021

hyunyong Oct 18, 2021

tvami Oct 18, 2021

tvami Oct 18, 2021

hyunyong Oct 18, 2021

tvami Oct 18, 2021

tvami commented Oct 18, 2021

cmsbuild commented Oct 18, 2021

cmsbuild commented Oct 18, 2021

makortel Oct 19, 2021

hyunyong Oct 19, 2021

cmsbuild commented Oct 19, 2021

tvami commented Oct 19, 2021

cmsbuild commented Oct 20, 2021

cmsbuild commented Oct 20, 2021

cmsbuild commented Oct 20, 2021

cmsbuild commented Oct 20, 2021

tvami commented Oct 20, 2021

cmsbuild commented Oct 20, 2021

tvami commented Oct 20, 2021

cmsbuild commented Oct 20, 2021

qliphy commented Oct 21, 2021

	MuonAlignmentInputSurveyDB(const DTGeometry* DTGeometry,
	MuonAlignmentInputSurveyDB(const DTGeometry* dtGeometry,

	const CSCGeometry* CSCGeometry,
	const CSCGeometry* cscGeometry,

	const GEMGeometry* GEMGeometry,
	const GEMGeometry* gemGeometry,

Migrate to esConsumes muon alignment #35719

Migrate to esConsumes muon alignment #35719

Conversation

hyunyong commented Oct 18, 2021 • edited Loading

PR description:

PR validation:

tvami left a 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 Oct 18, 2021

cmsbuild commented Oct 18, 2021

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

tvami commented Oct 18, 2021

cmsbuild commented Oct 18, 2021

cmsbuild commented Oct 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Oct 19, 2021

Comparison Summary

tvami commented Oct 19, 2021

cmsbuild commented Oct 20, 2021

cmsbuild commented Oct 20, 2021

Build

Clang Build

cmsbuild commented Oct 20, 2021

cmsbuild commented Oct 20, 2021

tvami commented Oct 20, 2021

cmsbuild commented Oct 20, 2021

Comparison Summary

tvami commented Oct 20, 2021

cmsbuild commented Oct 20, 2021

qliphy commented Oct 21, 2021

hyunyong commented Oct 18, 2021 •

edited

Loading