Muon arbitration #18321
Muon arbitration #18321
Conversation
|
A new Pull Request was created by @drkovalskyi for master. It involves the following packages: RecoMuon/MuonIdentification @perrotta, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here #13028 |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| @@ -637,6 +636,10 @@ void MuonIdProducer::produce(edm::Event& iEvent, const edm::EventSetup& iSetup) | |||
| } | |||
| } | |||
|
|
|||
| // muon arbitration | |||
| fillArbitrationInfo( outputMuons.get() ); | |||
There was a problem hiding this comment.
if (fillMatching_) was removed.
Does the flag fillMatching_ still make any sense or is it redundant?
There was a problem hiding this comment.
It's redundant. Didn't want to break anything removing it completely. Will remove it in future.
| @@ -637,6 +636,10 @@ void MuonIdProducer::produce(edm::Event& iEvent, const edm::EventSetup& iSetup) | |||
| } | |||
| } | |||
|
|
|||
| // muon arbitration | |||
| fillArbitrationInfo( outputMuons.get() ); | |||
| arbitrateMuons( outputMuons.get(), caloMuons.get() ); | |||
There was a problem hiding this comment.
should this always be done or should there be an option to not do it (for studies etc).
Recall that this module is used also to make muons as seeds for inside-out tracking iteration.
I can guess that for that case it's OK to have a larger number of options.
There was a problem hiding this comment.
similar question (as for earlyMuons used in iter tk seeding) for earlyDisplacedMuons.
And, also for muonsFromCosmics, muonsFromCosmics1Leg which are running both in pp and in proper cosmic data taking.
At least for proper cosmics it may be better to keep unarbitrated parts.
There was a problem hiding this comment.
I agree, providing an option can be of some use, but I don't think we need it. Point me to a single case where anyone is using non-arbitrated muons of any time intentionally.
There was a problem hiding this comment.
my example for iterative tracking seeding with "earlyMuons" still stands as an example of somewhat intentional use of non-arbitrated muons: seeding should better be loose
There was a problem hiding this comment.
IMO, it's easier to add an option and apply arbitration just to the default _muons_ than to prove there are no issues and no cost in all alternative reconstructions.
But we can go the other way and then this PR needs tests for all of these cases (cosmics, seeding, displaced muons).
There was a problem hiding this comment.
Agree. Will add this option. Do I need to submit a new PR in this case?
There was a problem hiding this comment.
I looked at implementing it - it's too much hassle. The module is used in tons of places (HLT) and cloned in even larger number of places. Providing it as an untracked parameter with a default is wrong, because it's important parameter. So I think it should stay as is, i.e. TrackerMuons will get arbitrated. I don't see why earlyMuons should not be arbitrated if they are TrackerMuons. All the zoo of muons that we have now won't be affected unless they are TrackerMuons, which is a specific algorithm and it can change.
There was a problem hiding this comment.
please use fillDescriptions to set a default value for a parameter.
The default can be overridden by specifying the parameter in a module instance where it's needed.
| } | ||
| } | ||
| } | ||
| muon++; |
There was a problem hiding this comment.
what happens here if muon = muons->erase(muon); above was done on the last element?
It looks like an undefined behavior. Better change to if (muon!=muons->end()) muon++;
There was a problem hiding this comment.
if it happens on the last element muon=muons->end() and for-loop exits. See no reason to modify it.
There was a problem hiding this comment.
erase happens on the last element, then in muon = muons->erase(muon); the muon is the same as the end() and in this line you will apply end()++, which is ill-defined and will not match the for loop condition
There was a problem hiding this comment.
There is a continue statement after muons->erase(muon). There won't be end()++ call.
| @@ -20,7 +20,7 @@ | |||
|
|
|||
| fillEnergy = cms.bool(True), | |||
| # OR | |||
| maxAbsPullX = cms.double(4.0), | |||
| maxAbsPullX = cms.double(3.0), | |||
There was a problem hiding this comment.
this will probably be noticeable in B=0T setup as a loss of 1/4 at lower p values. IIRC default track p in B=0T is 5 GeV. So, below this value there will be some signal loss.
... probably OK.
There was a problem hiding this comment.
Are you worried about 1GeV or less muons in B=0T case getting multiple scattering underestimated? It's non-issue in my opinion. 4sigma matching cut was very loose when it was introduced before Run1. All TrackerMuon selectors use at least 3 sigma cut or tighter. 4-sigma gives too many fakes in real data.
|
@drkovalskyi |
|
On 4/15/17 11:42 AM, drkovalskyi wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In RecoMuon/MuonIdentification/python/muons1stStep_cfi.py
<#18321 (comment)>:
> @@ -20,7 +20,7 @@
fillEnergy = cms.bool(True),
# OR
- maxAbsPullX = cms.double(4.0),
+ maxAbsPullX = cms.double(3.0),
Are you worried about 1GeV or less muons in B=0T case getting multiple
scattering underestimated? It's non-issue in my opinion. 4sigma matching
cut was very loose when it was introduced before Run1. All TrackerMuon
selectors use at least 3 sigma cut or tighter. 4-sigma gives too many
fakes in real data.
p = 1 GeV muons do not get through the calorimeter.
I was talking about those in range ~2.5 (rangeout) to 5 GeV.
For a 2.5 GeV muon, since track is reported as a 5 GeV, the pulls are
off by a factor of 2
and this cut becomes effectively a 1.5 sigma compared to the current 2
(4/2) for these muons
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#18321 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbjI-PFHi5t-OGsWyqGSLe8t48YXGks5rwQ-KgaJpZM4M6xA6>.
|
|
On 4/15/17 11:46 AM, drkovalskyi wrote:
There is a continue statement after muons->erase(muon). There won't be
end()++ call.
ah, thanks. I missed it
|
|
I think we can live with a small inefficiency of tighter matching for low pt muons at B=0. It's not a reason to waste space for real data. Beside, we have other algorithms to recover if TrackerMuon fails. |
|
On 4/15/17 11:49 AM, drkovalskyi wrote:
I think we can live with a small inefficiency of tighter matching for
low pt muons at B=0. It's not a reason to waste space for real data.
Beside, we have other algorithms to recover if TrackerMuon fails.
what are the other algorithms?
(standalone doesn't work here)
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#18321 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbt3p70-_ca0n4jS7f1ytQ4g8GmrGks5rwREtgaJpZM4M6xA6>.
|
|
In cosmic muons the number of matches now returns zero. Looks like something didn't get filled. e.g. from muon gun wf 10009.0
compared to that, the regular muons look OK these efficiency plots look suspicious (I haven't checked the definitions, just showing the plots): |
|
@drkovalskyi |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| @@ -637,6 +637,11 @@ void MuonIdProducer::produce(edm::Event& iEvent, const edm::EventSetup& iSetup) | |||
| } | |||
| } | |||
|
|
|||
| if (arbitrateTrackerMuons_){ | |||
| fillArbitrationInfo( outputMuons.get() ); | |||
There was a problem hiding this comment.
in case arbitrateTrackerMuons_ is true, what will happen with the call at L721
fillArbitrationInfo( outputMuons.get(), reco::Muon::TrackerMuon );
is the repeated call harmless or do we need an extra protection?
There was a problem hiding this comment.
There is a protection already in place in fillArbitrationInfo. There is a check if a segment has already been arbitrated: https://github.com/drkovalskyi/cmssw/blob/muon-arbitration/RecoMuon/MuonIdentification/plugins/MuonIdProducer.cc#L1037
|
Is there any open issues holding merging of this request? |
| ), | ||
|
|
||
| # tracker muon arbitration | ||
| arbitrateTrackerMuons = cms.bool(True) |
There was a problem hiding this comment.
The effect of this flag is that it ends up being set in all reconstructions (earlyMuons, a variety of cosmics etc).
Since the default is false, it stays unset in HLT, but even that is probably until the next re-parsing of confDB.
There was a problem hiding this comment.
I guess we can be "proactive" with this PR and have this change essentially universal.
The cases that will find some issues in the following more broad validation can have this flag set to false where needed.
There was a problem hiding this comment.
The option is there. If we ever find a case where it needs to be set false it's easy to do. I really doubt that it will ever happen.
|
The efficiency plots posted in #18321 (comment) are based on MC truth asocciation and apparently have SIM in the denominator. Slides sent in private emails stated that the impact is much smaller. It's unclear though if the difference is in the numerator definitions (e.g. global muons cover for some of the inefficiency in the post-arbitration tracker) or a sample like ttbar was not used to make final conclusions. |
|
What plot do you refer to? Where do you get all these numbers? |
|
On 4/25/17 4:00 AM, drkovalskyi wrote:
What plot do you refer to? Where do you get all these numbers?
The last plot in the thread posted on April 15
Here is a direct link
https://cloud.githubusercontent.com/assets/4676718/25066226/3d1c2fc4-21d5-11e7-8212-9474a2601a57.png
In my study of the arbitration inefficiency I clearly see that it's
often recovered by global muon algorithm. For muons at Pt>2GeV I see
3-4% efficiency loss, which is reduced to 1-2% overall efficiency loss
taking into account all different muon types.
Which samples were tested, just to have a good reference?
It would be nice to have a link to your slides/study posted in the PR
description for self-documentation.
Regarding J/psi, can you point me to a single analysis doing what you
said? As far as I can see people either use soft muons that rely on
arbitration or have their own Muon ID from scratch like Bs->mm.
B-tagging is also not using just any muon that they can find as far as I
can see.
The bottom line is that while indeed there can be some efficiency loss
that theoretically can be detected we have huge contribution of fake
muons from not using arbitration causing problems in physics analyses
(fake MET) and large amount of data stored in AOD, which doesn't fit
well in CMS budget anymore.
I only follow the argument about the data storage so far (which appears
to be about 2%).
I miss the point about effect on MET etc: on one hand you claim that all
analyses are using arbitrated muons,
on the other hand there is some huge contribution from not using
arbitration causing problems.
Where are the non-arbitrated muons used?
(and why can't that code simply start using arbitration)
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#18321 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbkPbhlXqJo2VZ1bpEJCVtQ7i7HErks5rzdI3gaJpZM4M6xA6>.
|
|
I think all these questions are addressed here: If we find later that we can recover efficiency somewhere and can afford it it's just a matter of change one flag from true to false. |
|
Thank you for the link. |
|
/RelValTTbarLepton_13/CMSSW_9_1_0_pre2-PU25ns_90X_upgrade2017_realistic_v20-v1/GEN-SIM-RECO |
|
On 4/25/17 11:58 AM, drkovalskyi wrote:
/RelValTTbarLepton_13/CMSSW_9_1_0_pre2-PU25ns_90X_upgrade2017_realistic_v20-v1/GEN-SIM-RECO
/RelValZMM_13/CMSSW_9_1_0_pre2-PU25ns_90X_upgrade2017_realistic_v20-v1/GEN-SIM-RECO
Thanks.
This should be well representative.
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#18321 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbhs-U0EnECWkveeCwhujkzxUGf3sks5rzkJxgaJpZM4M6xA6>.
|
|
What's holding this request from being merged? |
|
this PR has a strictly negative impact on downstream physics at a cost of gaining 1% in AOD size. My initial quick check pointed that there could be a significant loss in efficiency. More details later. |
|
In order to reproduce my numbers you need to do matching of muons to the generated muons. Otherwise you can observe a larger "inefficiency", which is simply fake reduction and not efficiency loss. |
|
On 4/27/17 9:41 AM, drkovalskyi wrote:
In order to reproduce my numbers you need to do matching of muons to the
generated muons. Otherwise you can observe a larger "inefficiency",
which is simply fake reduction and not efficiency loss.
That's somewhat clear.
The DQM plots that gave me concerns (posted on April 15) have
truth-level matching.
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#18321 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbmpCPyATzolhbc1o2s7NVMP73sw6ks5r0MVXgaJpZM4M6xA6>.
|
|
+1
TTbar PU35 (phase-1 setup; same as the PR reference slides) on 1K events. tracker muons efficiency The same test done in PU70 has basically the same results, to confirm that increasing PU is not a concern. VBF H->BB sample in phase-0 (2016) setup with PU35 on 600 events the same on PU70 (mind the larger tracking fake rates here compared to the phase-1 ttbar above) In PU200 (100 events only) no efficiency change is observed. The sample size is small though and apparently only signal muons are considered here. The impact of this PR on workflow resource use is significant only in the output collection size. There is a small effect reducing CPU use in modules downstream from the muonID, but since many non-arbitrated muons were already filtered out, the reduction is small (maybe 0.5% on the total reco, well within 1 sigma of typical comparison precision).
|
|
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 requires discussion in the ORP meeting before it's merged. @Muzaffar, @davidlange6, @smuzaffar |
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
this is a change in wf 10224 |
Enforced Tracker Muon arbitration during reconstruction to reduce number of fake muons produced during reconstruction.