[Phase-2] Fix usage of L1 egamma seeds in Phase-2 HLT #42791

SohamBhattacharya · 2023-09-14T15:59:41Z

This PR fixes the usage of L1 egamma seeds in Phase-2 HLT.
See details of the fix in these slides.

Tagging Phase-2 Egamma HLT contacts/validators: @ravindkv @skeshri

cmsbuild · 2023-09-14T16:07:59Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42791/36902

This PR adds an extra 40KB to repository

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

cmsbuild · 2023-09-14T16:29:04Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42791/36903

This PR adds an extra 44KB to repository

cmsbuild · 2023-09-14T16:29:32Z

A new Pull Request was created by @SohamBhattacharya for master.

It involves the following packages:

HLTrigger/Configuration (hlt)
HLTrigger/HLTfilters (hlt)
RecoHGCal/TICL (upgrade, reconstruction)

@Martin-Grunewald, @missirol, @cmsbuild, @AdrianoDee, @srimanob, @jfernan2, @mmusich, @mandrenguyen can you please review it and eventually sign? Thanks.
@youyingli, @missirol, @silviodonato, @felicepantaleo, @forthommel, @rovere, @Martin-Grunewald, @apsallid, @sobhatta, @lecriste, @hatakeyamak, @ebrondol, @sameasy this is something you requested to watch as well.
@rappoccio, @antoniovilela, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

mmusich · 2023-09-14T16:36:26Z

@cmsbuild, please test

cmsbuild · 2023-09-14T19:11:58Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9a6972/34765/summary.html
COMMIT: a96c90c
CMSSW: CMSSW_13_3_X_2023-09-14-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/42791/34765/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 5 lines from the logs
Reco comparison results: 12 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3348648
DQMHistoTests: Total failures: 3
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3348623
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
Checked 214 log files, 167 edm output root files, 50 DQM output files
TriggerResults: found differences in 4 / 48 workflows

SohamBhattacharya · 2023-09-14T20:00:49Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9a6972/34765/summary.html COMMIT: a96c90c CMSSW: CMSSW_13_3_X_2023-09-14-1100/el8_amd64_gcc11 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/42791/34765/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:
* You potentially removed 5 lines from the logs

* Reco comparison results: 12 differences found in the comparisons

* DQMHistoTests: Total files compared: 50

* DQMHistoTests: Total histograms compared: 3348648

* DQMHistoTests: Total failures: 3

* DQMHistoTests: Total nulls: 0

* DQMHistoTests: Total successes: 3348623

* DQMHistoTests: Total skipped: 22

* DQMHistoTests: Total Missing objects: 0

* DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)

* Checked 214 log files, 167 edm output root files, 50 DQM output files

* [TriggerResults](https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_13_3_X_2023-09-14-1100+9a6972/58880/triggerResults): found differences in 4 / 48 workflows

Changes to TriggerResults in 24834.0 and 24834.911 look expected.

mmusich

Changes to TriggerResults in 24834.0 and 24834.911 look expected.

I would be nice if also for the e/g objects a dedicated DQM / validation monitoring could be added (see standing issue #39362)

mmusich · 2023-09-15T07:00:53Z

HLTrigger/HLTfilters/plugins/L1TTkEleFilter.cc

@@ -147,8 +151,8 @@ bool L1TTkEleFilter::hltFilter(edm::Event& iEvent,
        passIsolation = true;
    }

-    if (offlinePt >= min_Pt_ && itkEle->eta() <= max_Eta_ && itkEle->eta() >= min_Eta_ && passQuality &&
-        passIsolation) {
+    if (offlinePt >= min_Pt_ && std::fabs(itkEle->eta()) < max_AbsEta1_ && std::fabs(itkEle->eta()) >= min_AbsEta1_ &&


elsewhere in this file std::abs is used

cmssw/HLTrigger/HLTfilters/plugins/L1TTkEleFilter.cc

Lines 148 to 149 in a96c90c

if (std::abs(itkEle->eta()) < etaBinsForIsolation_.at(etabin) and

std::abs(itkEle->eta()) > etaBinsForIsolation_.at(etabin - 1) and

I think that's the preferred option.

mmusich · 2023-09-15T07:02:18Z

HLTrigger/HLTfilters/plugins/L1TTkEmFilter.cc

@@ -147,7 +152,8 @@ bool L1TTkEmFilter::hltFilter(edm::Event& iEvent,
        passIsolation = true;
    }

-    if (offlinePt >= min_Pt_ && itkEm->eta() <= max_Eta_ && itkEm->eta() >= min_Eta_ && passQuality && passIsolation) {
+    if (offlinePt >= min_Pt_ && std::fabs(itkEm->eta()) < max_AbsEta1_ && std::fabs(itkEm->eta()) >= min_AbsEta1_ &&


elsewhere in this file std::abs is used

cmssw/HLTrigger/HLTfilters/plugins/L1TTkEmFilter.cc

Lines 181 to 182 in a96c90c

if (std::abs(itkEm->eta()) < etaBinsForIsolation_.at(etabin) and

std::abs(itkEm->eta()) > etaBinsForIsolation_.at(etabin - 1) and

I think that's the preferred option.

mmusich · 2023-09-15T07:02:32Z

HLTrigger/HLTfilters/plugins/L1TTkEmFilter.cc

@@ -177,7 +184,8 @@ bool L1TTkEmFilter::hltFilter(edm::Event& iEvent,
        passIsolation = true;
    }

-    if (offlinePt >= min_Pt_ && itkEm->eta() <= max_Eta_ && itkEm->eta() >= min_Eta_ && passQuality && passIsolation) {
+    if (offlinePt >= min_Pt_ && std::fabs(itkEm->eta()) <= max_AbsEta2_ && std::fabs(itkEm->eta()) >= min_AbsEta2_ &&


see above .

mmusich · 2023-09-15T07:03:24Z

RecoHGCal/TICL/plugins/SeedingRegionByL1.cc

-    if ((offlinePt < minPt_) || (std::abs(l1TrkEm.eta()) < minAbsEta_) || (std::abs(l1TrkEm.eta()) > maxAbsEta_) ||
-        (l1TrkEm.EGRef()->hwQual() != quality_)) {
-      continue;
+    if ((offlinePt < minPt_) || (std::abs(l1TrkEm.eta()) < minAbsEta_) || (std::abs(l1TrkEm.eta()) > maxAbsEta_)) {


this piece of code is a bit hard to read, I would suggest splitting conditions in dedicated booleans and then compose the final if accordingly (no strong push though).

…ionByL1 plugin

SohamBhattacharya · 2023-09-15T10:17:53Z

Changes to TriggerResults in 24834.0 and 24834.911 look expected.

I would be nice if also for the e/g objects a dedicated DQM / validation monitoring could be added (see standing issue #39362)

Indeed, that's at the top of our to-do list of post Annual Review tasks.

cmsbuild · 2023-09-15T10:27:54Z

Pull request #42791 was updated. @Martin-Grunewald, @missirol, @cmsbuild, @AdrianoDee, @srimanob, @jfernan2, @mmusich, @mandrenguyen can you please check and sign again.

mmusich · 2023-09-15T10:30:44Z

@cmsbuild, please test

mmusich · 2023-09-15T10:34:34Z

type bug-fix

swagata87 · 2023-09-15T12:15:31Z

Thank you Soham for the fix

cmsbuild · 2023-09-15T14:15:27Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9a6972/34778/summary.html
COMMIT: 4e1a47a
CMSSW: CMSSW_13_3_X_2023-09-15-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/42791/34778/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 16 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3348648
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3348620
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
Checked 214 log files, 167 edm output root files, 50 DQM output files
TriggerResults: found differences in 4 / 48 workflows

mmusich · 2023-09-15T14:20:09Z

+hlt

bug-fix PR is inline with initial description and follow-up review
a backport of this PR ([Phase-2] [backport] Fix usage of L1 egamma seeds in Phase-2 HLT #42792) is going to be needed in CMSSW_13_1_X in order to fulfill the goals of the Upgrade HLT group for the 2023 AR
as discussed in Add Validation and DQM to Phase2 HLT WF #39362 (comment) and following addition of e/g objects monitoring will be followed up.

jfernan2 · 2023-09-15T16:39:33Z

+1

srimanob · 2023-09-15T16:48:30Z

+upgrade

cmsbuild · 2023-09-15T16:49:00Z

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. @rappoccio, @antoniovilela, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2)

mmusich · 2023-09-18T06:56:58Z

@cms-sw/orp-l2 please take a look to this.

antoniovilela · 2023-09-18T09:56:55Z

+1

Fixed L1 egamma

161a0e2

cmsbuild added this to the CMSSW_13_3_X milestone Sep 14, 2023

cmsbuild added reconstruction-pending hlt-pending pending-signatures tests-pending orp-pending upgrade-pending code-checks-pending labels Sep 14, 2023

SohamBhattacharya mentioned this pull request Sep 14, 2023

[Phase-2] [backport] Fix usage of L1 egamma seeds in Phase-2 HLT #42792

Merged

cmsbuild added code-checks-rejected and removed code-checks-pending labels Sep 14, 2023

Implemented code checks and formats.

a96c90c

cmsbuild added code-checks-pending and removed code-checks-rejected labels Sep 14, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 14, 2023

cmsbuild added tests-started and removed tests-pending labels Sep 14, 2023

cmsbuild added tests-approved and removed tests-started labels Sep 14, 2023

mmusich reviewed Sep 15, 2023

View reviewed changes

Implemented PR comments and added flag to apply quality in SeedingReg…

4e1a47a

…ionByL1 plugin

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 15, 2023

cmsbuild added tests-started and removed tests-pending labels Sep 15, 2023

cmsbuild added the bug-fix label Sep 15, 2023

SohamBhattacharya mentioned this pull request Sep 15, 2023

Add Validation and DQM to Phase2 HLT WF #39362

Open

cmsbuild added tests-approved and removed tests-started labels Sep 15, 2023

cmsbuild added hlt-approved and removed hlt-pending labels Sep 15, 2023

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Sep 15, 2023

cmsbuild added fully-signed upgrade-approved and removed pending-signatures upgrade-pending labels Sep 15, 2023

cmsbuild added orp-approved and removed orp-pending labels Sep 18, 2023

cmsbuild merged commit f8caf8e into cms-sw:master Sep 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Phase-2] Fix usage of L1 egamma seeds in Phase-2 HLT #42791

[Phase-2] Fix usage of L1 egamma seeds in Phase-2 HLT #42791

SohamBhattacharya commented Sep 14, 2023 •

edited

Loading

cmsbuild commented Sep 14, 2023

cmsbuild commented Sep 14, 2023

cmsbuild commented Sep 14, 2023

mmusich commented Sep 14, 2023

cmsbuild commented Sep 14, 2023

SohamBhattacharya commented Sep 14, 2023

Comparison Summary

mmusich left a comment

mmusich Sep 15, 2023

mmusich Sep 15, 2023

mmusich Sep 15, 2023

mmusich Sep 15, 2023

SohamBhattacharya commented Sep 15, 2023

cmsbuild commented Sep 15, 2023

mmusich commented Sep 15, 2023

mmusich commented Sep 15, 2023

swagata87 commented Sep 15, 2023

cmsbuild commented Sep 15, 2023

mmusich commented Sep 15, 2023 •

edited

Loading

jfernan2 commented Sep 15, 2023

srimanob commented Sep 15, 2023

cmsbuild commented Sep 15, 2023

mmusich commented Sep 18, 2023

antoniovilela commented Sep 18, 2023

	if (std::abs(itkEle->eta()) < etaBinsForIsolation_.at(etabin) and
	std::abs(itkEle->eta()) > etaBinsForIsolation_.at(etabin - 1) and

	if (std::abs(itkEm->eta()) < etaBinsForIsolation_.at(etabin) and
	std::abs(itkEm->eta()) > etaBinsForIsolation_.at(etabin - 1) and

[Phase-2] Fix usage of L1 egamma seeds in Phase-2 HLT #42791

[Phase-2] Fix usage of L1 egamma seeds in Phase-2 HLT #42791

Conversation

SohamBhattacharya commented Sep 14, 2023 • edited Loading

cmsbuild commented Sep 14, 2023

cmsbuild commented Sep 14, 2023

cmsbuild commented Sep 14, 2023

mmusich commented Sep 14, 2023

cmsbuild commented Sep 14, 2023

Comparison Summary

SohamBhattacharya commented Sep 14, 2023

Comparison Summary

mmusich left a comment

Choose a reason for hiding this comment

mmusich Sep 15, 2023

Choose a reason for hiding this comment

mmusich Sep 15, 2023

Choose a reason for hiding this comment

mmusich Sep 15, 2023

Choose a reason for hiding this comment

mmusich Sep 15, 2023

Choose a reason for hiding this comment

SohamBhattacharya commented Sep 15, 2023

cmsbuild commented Sep 15, 2023

mmusich commented Sep 15, 2023

mmusich commented Sep 15, 2023

swagata87 commented Sep 15, 2023

cmsbuild commented Sep 15, 2023

Comparison Summary

mmusich commented Sep 15, 2023 • edited Loading

jfernan2 commented Sep 15, 2023

srimanob commented Sep 15, 2023

cmsbuild commented Sep 15, 2023

mmusich commented Sep 18, 2023

antoniovilela commented Sep 18, 2023

SohamBhattacharya commented Sep 14, 2023 •

edited

Loading

mmusich commented Sep 15, 2023 •

edited

Loading