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

New track algorithm names for HI muon RegIt #10653

Merged
merged 12 commits into from Aug 30, 2015

Conversation

echapon
Copy link
Contributor

@echapon echapon commented Aug 10, 2015

This PR involves the following small adjustments concerning regional iterative tracking (RegIt) for heavy ion muons:

  • Define new track algorithms in reco::TrackBase for the steps involved in HI muon RegIt. Among other things, this allows to fully follow the history of each track.
  • The "high purity" selectors for HI muon RegIt steps now set the highPurity flag to the tracks they find. Since this was not the case before (only "loose" or "tight" qualities were set), tracks found both by standard HI tracking and by HI muon RegIt "lost" their highPurity flag, thus effectively slightly biasing down the tracking efficiency close to muons.
  • The "tight" and "high purity" selectors for HI muon RegIt steps now inherit from the HI default selectors, not the pp ones, so that the selection of these qualities is more consistent. The "loose" selectors still inherit from pp, so as to be more permissive.

This PR is not expected to significantly change the reconstruction, except a tiny (sub-percent) increase of charged PF multiplicity in some events with high muon multiplicity.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @echapon (Emilien Chapon) for CMSSW_7_6_X.

New track algorithm names for HI muon RegIt

It involves the following packages:

DataFormats/TrackReco
RecoHI/HiMuonAlgos

@cmsbuild, @cvuosalo, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @MiheeJo, @jazzitup, @VinInn, @richard-cms, @yenjie, @kurtejung, @gpetruc, @istaslis, @mandrenguyen, @dgulhan, @yetkinyilmaz this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@istaslis
Copy link
Contributor

It needs also similar MVA selection for tight and highPurity as used in original HI tracking. In that case tracks will have uniform quality based on the reconstruction (primary, detached, pixelpair) rather then origin (initial or muon-regional).

@echapon
Copy link
Contributor Author

echapon commented Aug 10, 2015

ha, right. Let me make this change.

@cmsbuild
Copy link
Contributor

Pull request #10653 was updated. @cmsbuild, @cvuosalo, @slava77 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Aug 10, 2015

@VinInn @rovere Please confirm that the changes in the tracking DF are OK for you.

@slava77
Copy link
Contributor

slava77 commented Aug 10, 2015

@cmsbuild please test

@echapon Please clarify if this will need to be backported to 75X

@VinInn
Copy link
Contributor

VinInn commented Aug 10, 2015

I do not know what "algoSize = 46" will cause to algomask... (schema evolution needed???)
for sure HI people will need to use to_ullong() to access their bits...

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@@ -315,7 +316,8 @@
<class name="edm::Ref<std::vector<reco::TrackExtra>,reco::TrackExtra,edm::refhelper::FindUsingAdvance<std::vector<reco::TrackExtra>,reco::TrackExtra> >"/>
<class name="edm::RefVector<std::vector<reco::TrackExtra>,reco::TrackExtra,edm::refhelper::FindUsingAdvance<std::vector<reco::TrackExtra>,reco::TrackExtra> >"/>

<class name="reco::Track" ClassVersion="15">
<class name="reco::Track" ClassVersion="17">
Copy link
Contributor

Choose a reason for hiding this comment

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

what happened to version 16?
self answer: ROOT5 version (sic)

src = 'hiRegitMuDetachedTripletStepTrackCandidates'
)


import RecoTracker.FinalTrackSelectors.multiTrackSelector_cfi
import RecoHI.HiTracking.hiMultiTrackSelector_cfi
hiRegitMuDetachedTripletStepSelector = RecoTracker.FinalTrackSelectors.multiTrackSelector_cfi.multiTrackSelector.clone(
Copy link
Contributor

Choose a reason for hiding this comment

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

Selectors have to be cloned from RecoHI.HiTracking.hiMultiTrackSelector_cfi.hiMultiTrackSelector object. PR will be updated.

@echapon
Copy link
Contributor Author

echapon commented Aug 10, 2015

@slava77 yes I need to create a 75X PR too.

Also, unfortunately @istaslis told me my last commit was not enough for the MVA selection to work :( sorry about that... May I commit the missing stuff? I'll double check with him this time...

@slava77
Copy link
Contributor

slava77 commented Aug 24, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

Pull request #10653 was updated. @cmsbuild, @cvuosalo, @slava77 can you please check and sign again.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cvuosalo
Copy link
Contributor

Extended tests on 140.53 are in progress, but Jenkins shows no problem with additional dimuons. The other small changes still show up in the Jenkins DQM results:

jetpu

ratiojetpt
numtracks

@echapon
Copy link
Contributor Author

echapon commented Aug 25, 2015

Yes, I would say these small changes look fine.

@slava77
Copy link
Contributor

slava77 commented Aug 25, 2015

Is that dimuonHistograms InvMass plot now identical?

On 8/25/15 4:10 PM, Carl Vuosalo wrote:

Extended tests on 140.53 are in progress, but Jenkins shows no problem
with additional dimuons. The other small changes still show up in the
Jenkins DQM results:

jetpu
https://cloud.githubusercontent.com/assets/5736159/9479566/88c0ca80-4b43-11e5-8d0c-e13cda728ff5.png

ratiojetpt
https://cloud.githubusercontent.com/assets/5736159/9479564/88bd1f2a-4b43-11e5-838e-d7ce8d8cc9ee.png
numtracks
https://cloud.githubusercontent.com/assets/5736159/9479565/88bfe0f2-4b43-11e5-947f-cff53d8ef978.png


Reply to this email directly or view it on GitHub
#10653 (comment).

echapon added a commit to echapon/cmssw that referenced this pull request Aug 26, 2015
@cvuosalo
Copy link
Contributor

@echapon:
Extra dimuons are still appearing. With 70 events of workflow 140.53 vs. baseline CMSSW_7_6_0_pre3, DQM shows these differences:
dimu1
dimu2

For comparison, here are the LM dimuons that only slightly increase by about 1.6%:

dimu3
dimu4

@echapon
Copy link
Contributor Author

echapon commented Aug 28, 2015

Thank you for reporting this issue with the high mass di-tracker muons histogram. I had only looked at the default 20 events, which is why I had missed this.
I had a closer a look, and it turns out that the remaining additional high mass tracker muon pairs come from only two events, each with one high pt muon with the following characteristics:

  • not a PF muon
  • inner track with 10 tracker hits
  • 7 "layers with measurements"
  • "loose" track quality
  • found by the Regit pixel less step
  • 0 pixel hit
  • large dxy (|dxy|>2)

Because of the first and last two characteristics, I think that these problematic dimuon pairs will disappear at the analysis level (even assuming we will look at tracker muons, we require at least 1 pixel hit, and requiring a loose dxy cut like |dxy|<1 would kill these muons too). I would therefore say that we can live with these few high pt but poor quality fake muons.

@cvuosalo
Copy link
Contributor

+1

For #10653 a5518d4

Small adjustments for Heavy Ion regional iterative tracking.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_7_6_X_2015-08-24-1100 show a number of small differences, none significant. An extended test of workflow 140.53 with 70 events against baseline CMSSW_7_6_0_pre3 shows similar differences, plus additional dimuons, but, as discussed above, these differences are acceptable.
This PR does not appreciably increase the event size, and processing time increases by about 1.7%:

Baseline
Max VSIZ 3235.7 on evt 59 ; max RSS 2639.23 on evt 59
Time av 21.664 s/evt   max 102.632 s on evt 59
M1 Time av 21.6781 s/evt   max 102.632 s on evt 59

PR
Max VSIZ 3170.32 on evt 63 ; max RSS 2682.97 on evt 59
Time av 22.039 s/evt   max 103.606 s on evt 59
M1 Time av 22.0683 s/evt   max 103.606 s on evt 59

Another time measure:

Job total:  21.5366 s/ev ==> 21.9076 s/ev
Job total:  21.7374 s/ev ==> 22.1186 s/ev (first event excluded)

Largest apparent source of time increase:

  delta/mean delta/orJob     original                   new       module name
  ---------- ------------     --------                  ----       ------------
  +0.405369      +0.54%       229.74 ms/ev ->       346.55 ms/ev siStripMatchedRecHits

@cmsbuild
Copy link
Contributor

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

@davidlange6
Copy link
Contributor

+1

cmsbuild added a commit that referenced this pull request Aug 30, 2015
New track algorithm names for HI muon RegIt
@cmsbuild cmsbuild merged commit dd63445 into cms-sw:CMSSW_7_6_X Aug 30, 2015
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

9 participants