Build GE0 in the Muon Reco Geometry #32490

seulgi324 · 2020-12-15T05:54:58Z

PR description:

I copied the ME0 layer builders into MuonGEMDetLayerGeometryBuilder.cc, so that GE0 can be used by the muon reconstruction.
(https://indico.cern.ch/event/982928/contributions/4139669/attachments/2158615/3649556/Dec%208th%202020_ME0toGE0_Seulgi%20Kim.pdf)

@jshlee @watson-ij

PR validation:

I ran the matrix 30621.0 for D66 (ME0) and 31021.0 for D67 (GE0) both of which finished without failure. For D66, this change didn't affect the muon reconstruction. In D67, we now see that gem rechits from GE0 are used by muons.

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

Before submitting your pull requests, make sure you followed this checklist:

verify that the PR is really intended for the chosen branch
verify that changes follow CMS Naming, Coding, And Style Rules
verify that the PR passes the basic test procedure suggested in the CMSSW PR instructions

cmsbuild · 2020-12-15T06:01:34Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32490/20407

This PR adds an extra 16KB to repository

cmsbuild · 2020-12-15T06:01:59Z

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

It involves the following packages:

RecoMuon/DetLayers

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@bellan, @abbiendi, @Fedespring, @calderona, @HuguesBrun, @jhgoh, @trocino, @cericeci, @rociovilar this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

perrotta · 2020-12-15T06:36:53Z

please test

cmsbuild · 2020-12-15T09:28:13Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d24bb7/11659/summary.html
CMSSW: CMSSW_11_3_X_2020-12-14-2300/slc7_amd64_gcc900

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 36
DQMHistoTests: Total histograms compared: 2747284
DQMHistoTests: Total failures: 12
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2747249
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 35 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 153 log files, 37 edm output root files, 36 DQM output files

perrotta · 2020-12-15T09:09:23Z

RecoMuon/DetLayers/src/MuonGEMDetLayerGeometryBuilder.cc

  for (int endcap = -1; endcap <= 1; endcap += 2) {
    int iendcap = (endcap == 1) ? 0 : 1;  // +1: forward, -1: backward

+    // GE0
+    int station0 = 0;


This loop could be fully included in the next one, provided you start it from station = GEMDetId::minStationId0, and apply the few changes suggested below

perrotta · 2020-12-15T09:10:17Z

RecoMuon/DetLayers/src/MuonGEMDetLayerGeometryBuilder.cc

+      vector<int> rolls0, rings0, chambers0;
+      for (int ring = GEMDetId::minRingId; ring <= GEMDetId::maxRingId; ++ring)
+        rings0.push_back(ring);
+      for (int chamber = GEMDetId::minChamberId + 1; chamber <= GEMDetId::maxChamberId / 2; chamber++)


End the loop at GEMDetId::maxChamberId / 2 if station = GEMDetId::minStationId0

perrotta · 2020-12-15T09:10:40Z

RecoMuon/DetLayers/src/MuonGEMDetLayerGeometryBuilder.cc

+        rings0.push_back(ring);
+      for (int chamber = GEMDetId::minChamberId + 1; chamber <= GEMDetId::maxChamberId / 2; chamber++)
+        chambers0.push_back(chamber);
+      for (int roll = GEMDetId::minRollId + 1; roll <= GEMDetId::maxRollId - 6; ++roll)


End the loop at EMDetId::maxRollId - 6 if station = GEMDetId::minStationId0
(and maybe also explain why this "-6" comes out...)

perrotta · 2020-12-15T09:12:33Z

RecoMuon/DetLayers/src/MuonGEMDetLayerGeometryBuilder.cc

@@ -49,6 +65,68 @@ pair<vector<DetLayer*>, vector<DetLayer*> > MuonGEMDetLayerGeometryBuilder::buil
  return res_pair;
 }

+MuRingForwardLayer* MuonGEMDetLayerGeometryBuilder::buildLayer0(int endcap,


If I am not mistaken, buildLayer0 is identical to buildLayer and there is no need to duplicate them: the only difference is for bool isInFront, for which please see below.
All the comments below apply to buildLayer as well

perrotta · 2020-12-15T09:13:25Z

RecoMuon/DetLayers/src/MuonGEMDetLayerGeometryBuilder.cc

+  vector<const ForwardDetRing*> frontRings, backRings;
+
+  LogTrace(metname) << "Starting to Build Layer ";
+  for (std::vector<int>::iterator ring = rings.begin(); ring != rings.end() - 2; ring++) {


Why "-2"? A comment is deserved, at least
Better to fill the vector with only the needed rings (and not saying "-2" after having filled them)

perrotta · 2020-12-15T09:15:53Z

RecoMuon/DetLayers/src/MuonGEMDetLayerGeometryBuilder.cc

+
+        if (geomDet != nullptr) {
+          //ME0s do not currently have an arrangement of which are front and which are back, going to always return true
+          bool isInFront = true;


Suggested change

bool isInFront = true;

bool isInFront = (station == GEMDetId::minStationId0) or isFront(gemId);;

perrotta · 2020-12-15T09:17:01Z

RecoMuon/DetLayers/src/MuonGEMDetLayerGeometryBuilder.cc

+    // GE0
+    int station0 = 0;
+    for (int layer0 = GEMDetId::minLayerId + 1; layer0 <= GEMDetId::maxLayerId0; ++layer0) {
+      vector<int> rolls0, rings0, chambers0;


You can easily reserve the size of the vectors in advance

cmsbuild · 2020-12-16T12:52:20Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32490/20439

This PR adds an extra 16KB to repository

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

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32490/20439/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32490/20439/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2020-12-18T15:31:17Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32490/20502

This PR adds an extra 24KB to repository

cmsbuild · 2020-12-18T15:31:44Z

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

perrotta

Thank you for having removed all duplications with your latest update. Now I just suggest to further fix a typo, and then there are two very minor suggestions, that you can implement if you agree with them.
Well, three suggestions: please also remove the unneded inclusion of

#include "Utilities/BinningTools/interface/ClusterizingHistogram.h"

perrotta · 2020-12-22T14:26:53Z

RecoMuon/DetLayers/src/MuonGEMDetLayerGeometryBuilder.cc

-          chambers.push_back(chamber);
-        for (int roll = GEMDetId::minRollId + 1; roll <= GEMDetId::maxRollId; ++roll)
-          rolls.push_back(roll);
+      ForwardDetLayer* fowardLayer = nullptr;


Since you are introducing it, you can fix the typo: fowardLayer -> forwardLayer

perrotta · 2020-12-22T14:33:00Z

RecoMuon/DetLayers/src/MuonGEMDetLayerGeometryBuilder.cc

+      if (!backRings.empty() && !frontRings.empty() && st->station() != GEMDetId::minStationId0) {
+        fowardLayer = new MuRingForwardDoubleLayer(frontRings, backRings);
+      } else if (!frontRings.empty() && st->station() == GEMDetId::minStationId0) {
+        fowardLayer = new MuRingForwardLayer(frontRings);


Suggested change

if (!backRings.empty() && !frontRings.empty() && st->station() != GEMDetId::minStationId0) {

fowardLayer = new MuRingForwardDoubleLayer(frontRings, backRings);

} else if (!frontRings.empty() && st->station() == GEMDetId::minStationId0) {

fowardLayer = new MuRingForwardLayer(frontRings);

if (!frontRings.empty()) {

if (st->station() == GEMDetId::minStationId0)

forwardLayer = new MuRingForwardLayer(frontRings) ;

else if (!backRings.empty())

forwardLayer = new MuRingForwardDoubleLayer(frontRings, backRings);

}

perrotta · 2020-12-22T14:34:04Z

RecoMuon/DetLayers/src/MuonGEMDetLayerGeometryBuilder.cc

+    for (int layer = GEMDetId::minLayerId + 1; layer <= GEMDetId::maxLayerId0; ++layer) {
+      if (st->station() != GEMDetId::minStationId0 && layer > GEMDetId::maxLayerId)
+        break;


Suggested change

for (int layer = GEMDetId::minLayerId + 1; layer <= GEMDetId::maxLayerId0; ++layer) {

if (st->station() != GEMDetId::minStationId0 && layer > GEMDetId::maxLayerId)

break;

const int maxLayerId = st->station() == GEMDetId::minStationId0? GEMDetId::maxLayerId0 : GEMDetId::maxLayerId;

for (int layer = GEMDetId::minLayerId + 1; layer <= maxLayerId; ++layer) {

perrotta · 2020-12-22T14:54:26Z

While the geometry with ME0 (D66) is unchanged, in the the one with GE0 (D67) one can notice the effect of the inclusion of GE0 in the muon reconstruction, as expected and stated in the PR description.

The number of degrees of freedom in the muon fits increases:

with some limited propagation to the muon track parameters, e.g.

(examples from workflow 31021)

cmsbuild · 2020-12-24T10:00:34Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32490/20573

This PR adds an extra 16KB to repository

cmsbuild · 2020-12-24T10:00:59Z

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

perrotta · 2020-12-24T10:09:07Z

please test

cmsbuild · 2020-12-24T12:58:40Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d24bb7/11867/summary.html
CMSSW: CMSSW_11_3_X_2020-12-23-2300/slc7_amd64_gcc900

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-d24bb7/1325.81_TTbar_13_106Xv1NanoAODINPUT+TTbar_13_106Xv1NanoAODINPUT+NANOEDMMC2017_106XMiniAODv1+HARVESTNANOAODMC2017_106XMiniAODv1
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-d24bb7/136.8523_RunJetHT2018C_nanoULremini+RunJetHT2018C_nanoULremini+NANOEDM2018_106Xv2+HARVESTNANOAOD2018_106Xv2

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2716967
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2716938
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
Checked 156 log files, 37 edm output root files, 37 DQM output files

perrotta · 2020-12-24T15:07:25Z

+1

GE0 included in the Phase2 muon reco, when available
Jenkins tests pass, and show no differences in standard workflow
A few reconstructed muons updated with GE0 in the Phase2 workflows which include it

cmsbuild · 2020-12-24T15:07:47Z

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

qliphy · 2020-12-25T00:08:34Z

+1

Seulgi Kim added 2 commits December 14, 2020 19:44

added GE0 in MuonGEMDetLayerGeometryBuilder

4303bdb

added GE0 in MuonGEMDetLayerGeometryBuilder

11927d5

cmsbuild added this to the CMSSW_11_3_X milestone Dec 15, 2020

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Dec 15, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 15, 2020

cmsbuild added tests-started and removed tests-pending labels Dec 15, 2020

cmsbuild added tests-approved and removed tests-started labels Dec 15, 2020

perrotta reviewed Dec 15, 2020

View reviewed changes

added GE0 in MuonGEMDetLayerGeometryBuilder

01cf214

cmsbuild added code-checks-pending tests-pending and removed code-checks-approved tests-approved labels Dec 16, 2020

cmsbuild added code-checks-rejected and removed code-checks-pending labels Dec 16, 2020

added GE0 in MuonGEMDetLayerGeometryBuilder

c5def8d

cmsbuild added code-checks-pending and removed code-checks-rejected labels Dec 16, 2020

added GE0 in MuonGEMDetLayerGeometryBuilder

9dca606

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 18, 2020

perrotta reviewed Dec 22, 2020

View reviewed changes

added GE0 in MuonGEMDetLayerGeometryBuilder

f201f71

cmsbuild added code-checks-pending and removed code-checks-approved labels Dec 24, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 24, 2020

cmsbuild added tests-started and removed tests-pending labels Dec 24, 2020

cmsbuild added tests-approved and removed tests-started labels Dec 24, 2020

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Dec 24, 2020

cmsbuild added orp-approved and removed orp-pending labels Dec 25, 2020

cmsbuild merged commit bf306a6 into cms-sw:master Dec 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build GE0 in the Muon Reco Geometry #32490

Build GE0 in the Muon Reco Geometry #32490

seulgi324 commented Dec 15, 2020 •

edited

Loading

cmsbuild commented Dec 15, 2020

cmsbuild commented Dec 15, 2020

perrotta commented Dec 15, 2020

cmsbuild commented Dec 15, 2020

perrotta Dec 15, 2020

perrotta Dec 15, 2020

perrotta Dec 15, 2020

perrotta Dec 15, 2020

perrotta Dec 15, 2020

perrotta Dec 15, 2020

perrotta Dec 15, 2020

cmsbuild commented Dec 16, 2020

cmsbuild commented Dec 18, 2020

cmsbuild commented Dec 18, 2020

perrotta left a comment •

edited

Loading

perrotta Dec 22, 2020

perrotta Dec 22, 2020 •

edited

Loading

perrotta Dec 22, 2020

perrotta commented Dec 22, 2020

cmsbuild commented Dec 24, 2020

cmsbuild commented Dec 24, 2020

perrotta commented Dec 24, 2020

cmsbuild commented Dec 24, 2020

perrotta commented Dec 24, 2020

cmsbuild commented Dec 24, 2020

qliphy commented Dec 25, 2020

	bool isInFront = true;
	bool isInFront = (station == GEMDetId::minStationId0) or isFront(gemId);;

-      if (!backRings.empty() && !frontRings.empty() && st->station() != GEMDetId::minStationId0) {
-        fowardLayer = new MuRingForwardDoubleLayer(frontRings, backRings);
-      } else if (!frontRings.empty() && st->station() == GEMDetId::minStationId0) {
-        fowardLayer = new MuRingForwardLayer(frontRings);
+      if (!frontRings.empty()) {
+        if  (st->station() == GEMDetId::minStationId0)
+          forwardLayer =  new MuRingForwardLayer(frontRings) ;
+        else if (!backRings.empty())
+          forwardLayer = new MuRingForwardDoubleLayer(frontRings, backRings);
+      }

Build GE0 in the Muon Reco Geometry #32490

Build GE0 in the Muon Reco Geometry #32490

Conversation

seulgi324 commented Dec 15, 2020 • edited Loading

PR description:

PR validation:

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

cmsbuild commented Dec 15, 2020

cmsbuild commented Dec 15, 2020

perrotta commented Dec 15, 2020

cmsbuild commented Dec 15, 2020

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

cmsbuild commented Dec 16, 2020

cmsbuild commented Dec 18, 2020

cmsbuild commented Dec 18, 2020

perrotta left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perrotta Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perrotta commented Dec 22, 2020

cmsbuild commented Dec 24, 2020

cmsbuild commented Dec 24, 2020

perrotta commented Dec 24, 2020

cmsbuild commented Dec 24, 2020

Comparison Summary

perrotta commented Dec 24, 2020

cmsbuild commented Dec 24, 2020

qliphy commented Dec 25, 2020

seulgi324 commented Dec 15, 2020 •

edited

Loading

perrotta left a comment •

edited

Loading

perrotta Dec 22, 2020 •

edited

Loading