Follow-up of #32699 (Phase 2 Muon HLT) #32713

khaosmos93 · 2021-01-22T11:30:30Z

PR description:

Addressing comments in #32699

PR validation:

Phase-2 muon workflow

cmsbuild · 2021-01-22T11:36:51Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32713/20840

This PR adds an extra 24KB to repository

cmsbuild · 2021-01-22T11:37:26Z

A new Pull Request was created by @khaosmos93 (Minseok Oh) for master.

It involves the following packages:

HLTrigger/HLTfilters
HLTrigger/Muon
RecoMuon/L2MuonSeedGenerator

@perrotta, @Martin-Grunewald, @slava77, @cmsbuild, @fwyzard, @jpata can you please review it and eventually sign? Thanks.
@bellan, @abbiendi, @Fedespring, @Martin-Grunewald, @calderona, @HuguesBrun, @jhgoh, @trocino, @cericeci, @rociovilar 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

jpata · 2021-01-22T12:06:08Z

@cmsbuild please test

I'm posting my comments in #32713

perrotta · 2021-01-22T11:46:24Z

HLTrigger/HLTfilters/plugins/HLT2L1TkMuonL1TkMuonMuRefDR.cc


-  if (reco::deltaR(muRef1_eta, muRef1_phi, muRef2_eta, muRef2_phi) > minDR_)
+  if (reco::deltaR2(muRef1.first, muRef1.second, muRef2.first, muRef2.second) > minDR_ * minDR_)


Default for minDR_ is "-1"
Perhaps, you can define "minDR2" as input parameter (and let its default value as "-1")

@perrotta Thanks a lot for pointing it out!
Instead, would it make sense to just to the following without touching minDR_?

bool HLT2L1TkMuonL1TkMuonMuRefDR::computeDR(edm::Event& iEvent, l1t::TkMuonRef& r1, l1t::TkMuonRef& r2) const { if(minDR_ < 0.) return true; std::pair<float, float> muRef1 = convertEtaPhi(r1); std::pair<float, float> muRef2 = convertEtaPhi(r2);

Yes, even faster.

And maybe also

return (reco::deltaR2(muRef1.first, muRef1.second, muRef2.first, muRef2.second) > minDR_ * minDR_);

perrotta · 2021-01-22T11:53:33Z

RecoMuon/L2MuonSeedGenerator/plugins/BuildFile.xml

+    <use name="TrackingTools/KalmanUpdators"/>
+    <use name="TrackingTools/TrajectoryParametrization"/>
+    <use name="TrackingTools/TrajectoryState"/>
+    <use name="TrackingTools/TrackAssociator"/>


Isn't there anything that can be cleaned up from RecoMuon/L2MuonSeedGenerator/BuildFile.xml ?

perrotta · 2021-01-22T12:03:22Z

RecoMuon/L2MuonSeedGenerator/plugins/L2MuonSeedGeneratorFromL1TkMu.cc

+                              offseedTsos.globalPosition().eta(),
+                              offseedTsos.globalPosition().phi());
+      LogDebug(metlabel) << "   -- DR = " << newDr2 << std::endl;
+      if (newDr2 < dRcone * dRcone && newDr2 < bestDr2) {


Suggested change

if (newDr2 < dRcone * dRcone && newDr2 < bestDr2) {

if (newDr2 < bestDr2 && newDr2 < dRcone * dRcone) {

can avoid computing the product after the first iteration.

By the way, is it granted that dRcone is always >=0 here?

I made it return nullptr if dRcone < 0.

cmsbuild · 2021-01-22T14:16:46Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32713/20845

This PR adds an extra 28KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoMuon/L2MuonSeedGenerator/BuildFile.xml modified in PR(s): Clean various BuildFiles #32714

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

cmsbuild · 2021-01-26T10:13:30Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32713/20891

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoMuon/L2MuonSeedGenerator/BuildFile.xml modified in PR(s): Clean various BuildFiles #32714

cmsbuild · 2021-01-26T10:14:55Z

Pull request #32713 was updated. @perrotta, @Martin-Grunewald, @slava77, @cmsbuild, @fwyzard, @jpata can you please check and sign again.

fwyzard · 2021-01-26T10:15:20Z

please test

fwyzard · 2021-01-26T10:15:23Z

+1

cmsbuild · 2021-01-26T13:50:44Z

-1

Failed Tests: RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-abe294/12541/summary.html
COMMIT: 599868e
CMSSW: CMSSW_11_3_X_2021-01-25-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/32713/12541/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

4.764.76_ZMuSkim2012D+ZMuSkim2012D+HLTDSKIM2+RECODR1reHLT2+HARVESTDR1reHLT/step2_ZMuSkim2012D+ZMuSkim2012D+HLTDSKIM2+RECODR1reHLT2+HARVESTDR1reHLT.log

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 7 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2716961
DQMHistoTests: Total failures: 12
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2716926
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

jpata · 2021-01-26T16:31:19Z

+reconstruction

resign since 599868e (nothing major changed)
fixes the review items from New producers and filters for the Phase 2 Muon HLT #32699
tests do not show anything (code was not run in tests, failures are from DAS and not this PR)
this should be backported to 11_1_X

cmsbuild · 2021-01-26T16:31:46Z

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)

qliphy · 2021-01-27T05:01:30Z

+1

qliphy · 2021-01-27T05:05:46Z

merge

khaosmos93 added 3 commits January 22, 2021 11:11

addressing comments in PR cms-sw#32699

d44f06c

checks and format

1b949a2

const -> static constexpr

994f5b1

cmsbuild added this to the CMSSW_11_3_X milestone Jan 22, 2021

cmsbuild added code-checks-pending hlt-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jan 22, 2021

khaosmos93 mentioned this pull request Jan 22, 2021

New producers and filters for the Phase 2 Muon HLT #32699

Merged

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 22, 2021

khaosmos93 mentioned this pull request Jan 22, 2021

New producers and filters for the Phase 2 Muon HLT - Pending comments (#32699 and #32700) #32709

Closed

cmsbuild added tests-started and removed tests-pending labels Jan 22, 2021

perrotta reviewed Jan 22, 2021

View reviewed changes

more comments

ebbcbeb

cmsbuild added code-checks-pending tests-pending and removed code-checks-approved tests-started labels Jan 22, 2021

forgot to remove commented out lines

d2f34a8

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jan 22, 2021

code checks format

50aa2cf

cmsbuild added code-checks-pending hlt-pending pending-signatures reconstruction-pending tests-pending labels Jan 26, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 26, 2021

cmsbuild added hlt-approved tests-started and removed hlt-pending tests-pending labels Jan 26, 2021

cmsbuild added tests-rejected and removed tests-started labels Jan 26, 2021

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Jan 26, 2021

cmsbuild added orp-approved and removed orp-pending labels Jan 27, 2021

cmsbuild merged commit 9d6d658 into cms-sw:master Jan 27, 2021

khaosmos93 mentioned this pull request Jan 27, 2021

(11.1.X) Follow-up of #32700 (Phase 2 Muon HLT) #32751

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow-up of #32699 (Phase 2 Muon HLT) #32713

Follow-up of #32699 (Phase 2 Muon HLT) #32713

khaosmos93 commented Jan 22, 2021 •

edited

cmsbuild commented Jan 22, 2021

cmsbuild commented Jan 22, 2021

jpata commented Jan 22, 2021

perrotta Jan 22, 2021

khaosmos93 Jan 22, 2021

perrotta Jan 22, 2021

perrotta Jan 22, 2021

perrotta Jan 22, 2021

khaosmos93 Jan 22, 2021

cmsbuild commented Jan 22, 2021

cmsbuild commented Jan 26, 2021

cmsbuild commented Jan 26, 2021

fwyzard commented Jan 26, 2021

fwyzard commented Jan 26, 2021

cmsbuild commented Jan 26, 2021

jpata commented Jan 26, 2021 •

edited

cmsbuild commented Jan 26, 2021

qliphy commented Jan 27, 2021

qliphy commented Jan 27, 2021


		if (reco::deltaR(muRef1_eta, muRef1_phi, muRef2_eta, muRef2_phi) > minDR_)
		if (reco::deltaR2(muRef1.first, muRef1.second, muRef2.first, muRef2.second) > minDR_ * minDR_)

	if (newDr2 < dRcone * dRcone && newDr2 < bestDr2) {
	if (newDr2 < bestDr2 && newDr2 < dRcone * dRcone) {

Follow-up of #32699 (Phase 2 Muon HLT) #32713

Follow-up of #32699 (Phase 2 Muon HLT) #32713

Conversation

khaosmos93 commented Jan 22, 2021 • edited

PR description:

PR validation:

cmsbuild commented Jan 22, 2021

cmsbuild commented Jan 22, 2021

jpata commented Jan 22, 2021

perrotta Jan 22, 2021

Choose a reason for hiding this comment

khaosmos93 Jan 22, 2021

Choose a reason for hiding this comment

perrotta Jan 22, 2021

Choose a reason for hiding this comment

perrotta Jan 22, 2021

Choose a reason for hiding this comment

perrotta Jan 22, 2021

Choose a reason for hiding this comment

khaosmos93 Jan 22, 2021

Choose a reason for hiding this comment

cmsbuild commented Jan 22, 2021

cmsbuild commented Jan 26, 2021

cmsbuild commented Jan 26, 2021

fwyzard commented Jan 26, 2021

fwyzard commented Jan 26, 2021

cmsbuild commented Jan 26, 2021

RelVals-INPUT

Comparison Summary

jpata commented Jan 26, 2021 • edited

cmsbuild commented Jan 26, 2021

qliphy commented Jan 27, 2021

qliphy commented Jan 27, 2021

khaosmos93 commented Jan 22, 2021 •

edited

jpata commented Jan 26, 2021 •

edited