adding duplicate removal and implimenting L1 quality cuts #33026
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33026/21318
|
|
A new Pull Request was created by @Sam-Harper (Sam Harper) for master. It involves the following packages: HLTrigger/HLTfilters @Martin-Grunewald, @cmsbuild, @fwyzard can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
| bool passMuQual(const l1t::TkMuon& muon) { | ||
| if (muon.muonDetector() == 3) { | ||
| int quality = muon.quality(); | ||
| if (quality == 11 || quality == 13 || quality == 14 || quality == 15) { |
There was a problem hiding this comment.
are these values defined anywhere, e.g. in some L1T package ?
There was a problem hiding this comment.
no idea. I freely admit these magic numbers are not great and should move. What we could do is make it configurable with a vector "allowed qualities".
Basically I read the twiki and applied the cuts? That is extent of my understanding here :)
There was a problem hiding this comment.
Indeed, I haven't been able to find any definition that one could reuse in the L1 software
| for (const auto& exist : existing) { | ||
| //it is our understanding that there is an exact eta phi match | ||
| //and we should not be concerned with numerical precision | ||
| if (reco::deltaR2(*muon, *exist) <= 0) { |
There was a problem hiding this comment.
are the duplicate muons always from the same subsystem (barrel/overlap/forward) ?
or are the coordinates exactly the same also for muons from different subsystems ?
There was a problem hiding this comment.
I dont know. I do know the cut listed on the twiki is deltaR>0
| applyQuality_ = iConfig.getParameter<bool>("applyQuality"); | ||
| doDupRemoval_ = iConfig.getParameter<bool>("doDupRemoval"); |
There was a problem hiding this comment.
I'd prefer applyQualityCuts and applyDuplicateRemoval for consistency among the new parameter names
not a strong opinion, though
There was a problem hiding this comment.
I dont really mind. I'm happy to make that change.
|
-1 Failed Tests: RelVals-INPUT
RelVals-INPUT
Comparison SummarySummary:
|
|
@cms-sw/hlt-l2 the DAS errors are not related to this PR (they have been 'fixed' by #33020 ) |
|
+1 |
|
This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). 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) |
|
Hi Martin, thanks for the speedy signing but I still need to address Andrea's comments which I will do shortly |
|
-1 |
cmsbuild
added
code-checks-pending
hlt-pending
tests-pending
and removed
code-checks-approved
hlt-rejected
tests-rejected
labels
|
please test |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33026/21332
|
|
Pull request #33026 was updated. @Martin-Grunewald, @fwyzard can you please check and sign again. |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-974298/13202/summary.html Comparison SummarySummary:
|
|
+1 |
cmsbuild
added
fully-signed
hlt-approved
and removed
hlt-pending
pending-signatures
labels
|
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) |
|
+1 |
PR description:
This PR adds the l1 muon quality selection and l1 muon duplicate removal
PR validation:
On 10000 MB events rate goes from 87+/-16kHz to 24kHz +/-8kHz for tripple mu 7_5_5 which now matches the L1 reference (https://twiki.cern.ch/twiki/bin/view/CMS/PhaseIIL1TriggerMenuTools)