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

GEM and ME0 segments into trackerMuon #14833

Merged
merged 29 commits into from Jul 14, 2016

Conversation

jshlee
Copy link
Contributor

@jshlee jshlee commented Jun 9, 2016

This PR adds GemMuon and Me0Muon as well as adding GEMSegments and ME0Segments into trackerMuons.

TAMuonSegmentMatch and TrackAssociatorParameters

  • added GEM segments
  • added ME0 segments

MuonIdProducer

  • added isGEMMuon function
  • added isME0Muon function
  • save isME0Muon and isGEMMuon with trackerMuons

muon.h

  • added numberOfChambersNoGEM function
  • added numberOfChambersNoME0 function
  • added isGEMMuon function
  • added isME0Muon function

MuonChamberMatch, MuonDetIdAssociator, DetIdInfo, TAMuonChamberMatch, TrackDetectorAssociator

  • added ME0 and GEM

ME0SegmentBuilder

  • fix chamberId()

MuonME0DetLayerGeometryBuilder

  • bug fix, no layer 0

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 9, 2016

A new Pull Request was created by @jshlee (Jason Lee) for CMSSW_8_1_X.

It involves the following packages:

DataFormats/MuonReco
RecoLocalMuon/GEMSegment
RecoMuon/DetLayers
RecoMuon/MuonIdentification
TrackingTools/TrackAssociator

@cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@battibass, @makortel, @abbiendi, @GiacomoSguazzoni, @jhgoh, @VinInn, @bellan, @HuguesBrun, @rovere, @amagitte, @gpetruc, @trocino, @dgulhan, @bachtis, @rociovilar this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@jshlee
Copy link
Contributor Author

jshlee commented Jun 9, 2016

@calabria and @kpedro88 - PR for GEMMuon and ME0Muon

@kpedro88
Copy link
Contributor

kpedro88 commented Jun 9, 2016

@jshlee - reminder: at some point, we need a long-term fix for MuonSegFit:
https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_X/RecoLocalMuon/GEMSegment/plugins/MuonSegFit.cc#L310

@slava77
Copy link
Contributor

slava77 commented Jun 9, 2016

@jshlee
is there a presentation/slides in muon POG that describe the changes?
please add to the PR description

@kpedro88
Copy link
Contributor

kpedro88 commented Jun 9, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 9, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/13444/console

@slava77
Copy link
Contributor

slava77 commented Jun 9, 2016

Can the isRPC be renamed to isGEMRPC so that we don't generate a new type?

@slava77
Copy link
Contributor

slava77 commented Jun 9, 2016

also, please add some discussion what was already done in
#13308
and is now present here and what was changed.

@jshlee
Copy link
Contributor Author

jshlee commented Jun 9, 2016

@slava77 - no yet for muon POG, we will give one soon, but due to the ECFA deadlines we wanted to go ahead with this first. I'm not sure what you mean by isRPC.

@kpedro88 - yes, will fix this soon.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 7, 2016

Pull request #14833 was updated. @perrotta, @cmsbuild, @cvuosalo, @fwyzard, @dmitrijus, @Martin-Grunewald, @slava77, @vanbesien, @davidlange6 can you please check and sign again.

@jshlee
Copy link
Contributor Author

jshlee commented Jul 7, 2016

@slava77 - I've fixed this and tested it with workflow 10624. Please test again.

@slava77
Copy link
Contributor

slava77 commented Jul 7, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 7, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/13935/console

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 7, 2016

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 7, 2016

@Martin-Grunewald
Copy link
Contributor

+1

@dmitrijus
Copy link
Contributor

+1

@@ -46,7 +48,8 @@ namespace reco {

DTRecSegment4DRef dtSegmentRef;
CSCSegmentRef cscSegmentRef;

GEMSegmentRef gemSegmentRef;
ME0SegmentRef me0SegmentRef;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this, together with the use case pattern is becoming more and more bloated.
std::vector<reco::MuonSegmentMatch> segmentMatches can have a value only among dtSegmentRef or cscSegmentRef, while
std::vector<reco::MuonSegmentMatch> gemMatches can have a meaningful value only for gemSegmentRef
... and inside the MuonChamberMatch only one of these vectors has a meaning.

This is probably still OK considering the most frequent access patterns: actual segments are used mainly in detailed detector analysis (truth matching and alike).
The alternatives will likely lead to polymorphic generalized more effective storage, but, very likely, access will require casting back to specific classes.

@slava77
Copy link
Contributor

slava77 commented Jul 13, 2016

@jshlee @calabria @trocino
after the last updates we now have more muons in the muon collections. As expected, they are not modifying the trackerMuons set. In the (no pileup) QCD dijet sample pt 600to800 (wf 10626.0) the total number of muons about doubles (from 15 to 29).

The additional muons contribute also to the earlyMuons collection and as a result affect tracking
e.g. 10626.0
all_sign728vsorig-not2ch_qcd600to800in13tev2023wf10626p0c_recotracks_generaltracks__reco_obj_algo
Since this is only in in-out iteration, the effect is going to be rather small, but will still likely enhance low quality track reconstruction (fakes and not).
For this workfow, the effect is mainly in the GEM direction (I didn't notice changes in ME0 direction).

If you want to decouple the new tracker muon types more completely, the earlyMuons should be modified to not use GEM and ME0.

I don't think it matters much for performance, but I wanted to get a clarification what was the intended behavior (fully decoupled, just new muons, or integrated already in the muon-driven tracking iterations).

@jshlee
Copy link
Contributor Author

jshlee commented Jul 13, 2016

@slava77 - The idea was to go fully integrated already in the muon-driven tracking iterations, but not for identification.

@slava77
Copy link
Contributor

slava77 commented Jul 13, 2016

+1

for #14833 4690943

  • code changes are in line with the description and the follow up review/discussion. Only phase2 (and not yet existing run3) workflows are affected.
  • jenkins tests pass and show changes only in 2023 workflow
  • local tests with more 2023 workflows show expected behavior: new muons appear with GEM or RPC types and contribute to inside-out tracking iteration, but not to anything else from the _muons_ collection. On-disk increase is more significant in jetty events, expected for the loose matching definitions. On-disk increase of the reco::Muon size is negligible for non-phase-2 workflows (only empty vectors data in the new data members). CPU increase in mostly-real-muon samples is small; expected to be more significant with pileup or in high-pt jets (I'm guessing it to be less than a per cent in high-PU, even without HGCAL).

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_1_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar

@kpedro88
Copy link
Contributor

@davidlange6 please merge for pre9 if possible

@davidlange6 davidlange6 merged commit 11a8c31 into cms-sw:CMSSW_8_1_X Jul 14, 2016
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.

None yet