Separate Algorithms for GE0 and GE1/1 GE2/1 Segments #32972

watson-ij · 2021-02-23T11:46:15Z

PR description:

For ME0, the RU segment builder was default while GEMSegmentAlgorithm is used for the other GEM stations. This PR allows separate GE0 and other GEM algorithms, and defaults GE0 to use the RU algorithm, and GE1/1 / GE2/1 to use the GEMSegmentAlgorithm, as is the default for the ME0/GEM setup.

@jshlee

PR validation:

We checked the performance of the GE0 RU algorithm and found it to work as for ME0. I also ran runTheMatrix with workflow 34621.0 and confirmed that both station 0 and 1/2 segments are being created properly.

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 · 2021-02-23T11:50:51Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32972/21210

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-32972/21210/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32972/21210/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2021-02-23T12:03:54Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32972/21211

This PR adds an extra 16KB to repository

cmsbuild · 2021-02-23T12:04:13Z

A new Pull Request was created by @watson-ij (Ian J. Watson) for master.

It involves the following packages:

RecoLocalMuon/GEMSegment

@perrotta, @kpedro88, @cmsbuild, @srimanob, @slava77, @jpata can you please review it and eventually sign? Thanks.
@jshlee, @bellan, @jhgoh 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

srimanob · 2021-02-23T12:12:08Z

Please test

srimanob · 2021-02-23T12:20:26Z

@watson-ij
Could you please provide material or plots as a result of this PR? For example, what do you expect to see when replacing GEMSegmentAlgorithm (current) with RU (introduce in this PR)? Will it be spotted using DQM?

This is to complete the documentation for the PR. Thanks.

watson-ij · 2021-02-23T12:28:59Z

@seulgi324, do you have a link to a presentation showing the difference?

watson-ij · 2021-02-23T12:57:20Z

@watson-ij
Could you please provide material or plots as a result of this PR? For example, what do you expect to see when replacing GEMSegmentAlgorithm (current) with RU (introduce in this PR)? Will it be spotted using DQM?

This is to complete the documentation for the PR. Thanks.

GEMSegments aren't currently tracked by DQM, so they won't show up directly, but it could have an impact on the muons. We saw that the default algorithm produces a lot of segments with only a few rechits, whereas the RU algorithm requires at least 4. The RU algorithm is also finding more segments with 6 rechits as is nominal for ME0.

perrotta · 2021-02-23T13:01:15Z

RecoLocalMuon/GEMSegment/plugins/GEMSegmentBuilder.cc


  // Ask factory to build this algorithm, giving it appropriate ParameterSet
-  algo = GEMSegmentBuilderPluginFactory::get()->create(algoName, segAlgoPSet);
+  segAlgo = GEMSegmentBuilderPluginFactory::get()->create(segAlgoName, segAlgoPSet);
+  // Ask factory to build this algorithm, giving it appropriate ParameterSet


Having twice the same comment is probably redundant...
If you remove one, then in the other one: "this algorithm, giving it" -> "these algorithms, giving them"

Yep, updated

cmsbuild · 2021-02-23T14:39:50Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32972/21218

This PR adds an extra 16KB to repository

cmsbuild · 2021-02-23T14:40:13Z

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

srimanob · 2021-02-23T15:07:10Z

Please test

watson-ij · 2021-02-23T16:50:44Z

Could you please provide material or plots as a result of this PR?

To complete this: @seulgi324 gave a talk today showing the rechit distribution differences between the default and RU algorithms, in particular, showing the increase in 6-hit segments I mentioned earlier.

https://indico.cern.ch/event/1010727/contributions/4241679/attachments/2195182/3711306/Feb%2023rd%202021_ME0toGE0_Seulgi%20Kim.pdf

cmsbuild · 2021-02-23T17:43:09Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-86221f/13049/summary.html
COMMIT: bea91b3
CMSSW: CMSSW_11_3_X_2021-02-22-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/32972/13049/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: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2750983
DQMHistoTests: Total failures: 12
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2750948
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 36 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 156 log files, 37 edm output root files, 37 DQM output files

perrotta · 2021-02-27T16:58:40Z

The RU segment builder is implemented for the GE0, instead of the GEMSegmentAlgorithm which is used for all other GEM stations. The RU segment builder is what was originally implemented for ME0, and the GEMSegmentAlgorithm replaced it for GE0 in previous #31480: but it turned out to be sub-optimal for the GE0 station (something which was already possibly anticipated in the comments of #31480).

In the presentation at the GEM DPG linked here differences (expected) are reported for the number of segments and recHits. These differences cannot likely be noticed in the low stat Phase2 validations available in the automatic PR tests because of the low statistics: it looks like that they are however endorsed by the GEM DPG, something which was already possibly anticipated in the comments of #31480.

As such, the PR could be considered suitable for being integrated in CMSSW.

At page 6 of the same presentation there is however a note about "an issue with ME0": could you please elaborate further about it, and specify with some more detail what such an "issue" is, and whether you plan/need to fix it already in this PR?

watson-ij · 2021-03-02T04:23:12Z

At page 6 of the same presentation there is however a note about "an issue with ME0": could you please elaborate further about it, and specify with some more detail what such an "issue" is, and whether you plan/need to fix it already in this PR?

@perrotta, the issue referred to there was that the ME0 RecHit algorithm never implemented clustering, so that we have a rechit for every digi rather than clustering the contiguous digis into a single rechit, causing duplicate segments. This is already fixed for GE0 by virtue of the fact that the GEMRecHit algorithm includes a clusterizer, so GE0 digis are also clustered so we don't get unnecessary duplicates.

perrotta · 2021-03-02T13:22:51Z

+1

RU segment builder implemented for the GE0 instead of the GEMSegmentAlgorithm, which is used for the other GEM stations.
Jenkins tests pass and show no differences (likely because of their low stat)
Differences in the number of segments and recHits are visible in some large stat validation shown in the slides attached: they go in the expected direction

silviodonato · 2021-03-08T10:04:18Z

do you have any comments @cms-sw/upgrade-l2 ?

srimanob · 2021-03-08T10:49:08Z

+Upgrade

cmsbuild · 2021-03-08T10:49:32Z

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)

silviodonato · 2021-03-08T17:17:18Z

+1

watson-ij added 2 commits February 23, 2021 18:19

Separate algorithms for GE0 and GEx/1 segment building

ee25e36

fix variable names

17b2490

cmsbuild added this to the CMSSW_11_3_X milestone Feb 23, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending upgrade-pending labels Feb 23, 2021

cmsbuild added code-checks-rejected and removed code-checks-pending labels Feb 23, 2021

style

0ad8e1b

cmsbuild added code-checks-pending and removed code-checks-rejected labels Feb 23, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 23, 2021

cmsbuild added tests-started and removed tests-pending labels Feb 23, 2021

perrotta reviewed Feb 23, 2021

View reviewed changes

Fix comments

721d8c0

cmsbuild added code-checks-pending and removed code-checks-approved tests-started labels Feb 23, 2021

cmsbuild added the tests-pending label Feb 23, 2021

Fix comment

bea91b3

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 23, 2021

cmsbuild added tests-started and removed tests-pending labels Feb 23, 2021

cmsbuild added tests-approved and removed tests-started labels Feb 23, 2021

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Mar 2, 2021

cmsbuild added fully-signed upgrade-approved and removed pending-signatures upgrade-pending labels Mar 8, 2021

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

cmsbuild merged commit 7da081e into cms-sw:master Mar 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate Algorithms for GE0 and GE1/1 GE2/1 Segments #32972

Separate Algorithms for GE0 and GE1/1 GE2/1 Segments #32972

watson-ij commented Feb 23, 2021

cmsbuild commented Feb 23, 2021

cmsbuild commented Feb 23, 2021

cmsbuild commented Feb 23, 2021

srimanob commented Feb 23, 2021

srimanob commented Feb 23, 2021

watson-ij commented Feb 23, 2021

watson-ij commented Feb 23, 2021

perrotta Feb 23, 2021

watson-ij Feb 23, 2021

cmsbuild commented Feb 23, 2021

cmsbuild commented Feb 23, 2021

srimanob commented Feb 23, 2021

watson-ij commented Feb 23, 2021

cmsbuild commented Feb 23, 2021

perrotta commented Feb 27, 2021 •

edited

watson-ij commented Mar 2, 2021

perrotta commented Mar 2, 2021

silviodonato commented Mar 8, 2021

srimanob commented Mar 8, 2021

cmsbuild commented Mar 8, 2021

silviodonato commented Mar 8, 2021

Separate Algorithms for GE0 and GE1/1 GE2/1 Segments #32972

Separate Algorithms for GE0 and GE1/1 GE2/1 Segments #32972

Conversation

watson-ij commented Feb 23, 2021

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 Feb 23, 2021

cmsbuild commented Feb 23, 2021

cmsbuild commented Feb 23, 2021

srimanob commented Feb 23, 2021

srimanob commented Feb 23, 2021

watson-ij commented Feb 23, 2021

watson-ij commented Feb 23, 2021

perrotta Feb 23, 2021

Choose a reason for hiding this comment

watson-ij Feb 23, 2021

Choose a reason for hiding this comment

cmsbuild commented Feb 23, 2021

cmsbuild commented Feb 23, 2021

srimanob commented Feb 23, 2021

watson-ij commented Feb 23, 2021

cmsbuild commented Feb 23, 2021

Comparison Summary

perrotta commented Feb 27, 2021 • edited

watson-ij commented Mar 2, 2021

perrotta commented Mar 2, 2021

silviodonato commented Mar 8, 2021

srimanob commented Mar 8, 2021

cmsbuild commented Mar 8, 2021

silviodonato commented Mar 8, 2021

perrotta commented Feb 27, 2021 •

edited