[14.0.X] store the hltobjects for Mass50 triggers #44746

pinchunchou · 2024-04-15T22:02:04Z

PR description:

We've encountered an issue with the Mass50 HLT triggers for double electrons (Ele15Ele10GsfMass50, DoubleEle10GsfMass50, and DoubleEle15GsfMass50) in both ppref and HI HLT menus. These HLT triggers are not storing any information in the hltobject trees, even though the events are being triggered. After investigating, we have identified that the problem lies with HLTEgammaCombMassFilter.

I have made some modifications to HLTEgammaCombMassFilter, and now the information is being stored in hltobject.

See slides: https://twiki.cern.ch/twiki/pub/CMS/HLT_HIon_Run3/pinchun20230928.pdf (P.8)

cmsbuild · 2024-04-15T22:02:32Z

A new Pull Request was created by @pinchunchou for CMSSW_14_0_X.

It involves the following packages:

HLTrigger/Egamma (hlt)

@Martin-Grunewald, @mmusich, @cmsbuild can you please review it and eventually sign? Thanks.
@silviodonato, @Martin-Grunewald, @missirol this is something you requested to watch as well.
@rappoccio, @sextonkennedy, @antoniovilela you are the release manager for this.

cms-bot commands are listed here

Backported from store the hltobjects for Mass50 triggers #44778

cmsbuild · 2024-04-15T22:02:33Z

cms-bot internal usage

Martin-Grunewald · 2024-04-16T04:22:50Z

You need to provide a PR to master branch (currently 14_1_X) first, and then link this PR to be its backport to 14_0.

mmusich · 2024-04-16T07:18:25Z

HLTrigger/Egamma/plugins/HLTEgammaCombMassFilter.cc

@@ -45,6 +47,35 @@ bool HLTEgammaCombMassFilter::hltFilter(edm::Event& iEvent,
                                        trigger::TriggerFilterObjectWithRefs& filterproduct) const {
  //right, issue 1, we dont know if this is a TriggerElectron, TriggerPhoton, TriggerCluster (should never be a TriggerCluster btw as that implies the 4-vectors are not stored in AOD)


Does this comment still apply after the changes proposed here?

I think this problem should already be solved after we applied the proposed change, but one can verify if this is really solved or not.

If it does, then please remove the comment.

Sure, I have removed the comment. Thanks for the reminder!

mmusich · 2024-04-17T12:15:47Z

@pinchunchou can you please address the comments?

pinchunchou · 2024-04-17T17:44:12Z

You need to provide a PR to master branch (currently 14_1_X) first, and then link this PR to be its backport to 14_0.

Thanks for the reminder. I will do this

cmsbuild · 2024-04-18T17:48:08Z

Pull request #44746 was updated. @cmsbuild, @mmusich, @Martin-Grunewald can you please check and sign again.

cmsbuild · 2024-04-18T17:57:25Z

Pull request #44746 was updated. @mmusich, @cmsbuild, @Martin-Grunewald can you please check and sign again.

pinchunchou · 2024-04-18T17:58:36Z

You need to provide a PR to master branch (currently 14_1_X) first, and then link this PR to be its backport to 14_0.

Thanks for the reminder. I will do this

PR to master branch: #44778

mmusich · 2024-04-18T18:02:55Z

backport of #44778

mmusich · 2024-04-18T18:03:14Z

type egamma, bug-fix

mmusich · 2024-04-18T18:03:38Z

@cmsbuild, please test

missirol · 2024-04-18T18:21:05Z

HLTrigger/Egamma/plugins/HLTEgammaCombMassFilter.cc

@@ -35,6 +36,7 @@ void HLTEgammaCombMassFilter::fillDescriptions(edm::ConfigurationDescriptions& d
  makeHLTFilterDescription(desc);
  desc.add<edm::InputTag>("firstLegLastFilter", edm::InputTag("firstFilter"));
  desc.add<edm::InputTag>("secondLegLastFilter", edm::InputTag("secondFilter"));
+  desc.add<edm::InputTag>("l1EGCand", edm::InputTag("hltL1IsoRecoEcalCandidate"));


Since the default value of the tag is not realistic, and the PR has no changes to the python configs, I would naively expect some test to fail because of a misconfigured InputTag. Or not ?

scram b runtests passed without failures - if there are python configs where this parameter should be added, they are not used in testing. Are the configs where the module is called such as HLTrigger/Configuration/test/OnLine_HLT_FULL.py are expected to need changes?

fillDescriptions inserts values/parameters if they are missing in the py config, so no changes needed there.
However, if the default of a new parameter is not realistic, then it is either not used or misused. Please check!

Hi @Martin-Grunewald , since it passes tests its unclear to me what is meant by the current value is unrealistic, could you clarify?
BTW if I remove that line (L39), when doing HLT emulation we would have error as below:

----- Begin Fatal Exception 17-Jun-2024 07:35:03 CEST-----------------------
An exception of category 'Configuration' occurred while
[0] Constructing the EventProcessor
[1] Constructing module: class=HLTEgammaCombMassFilter label='hltDoubleEle10Mass50PPRefFilter'
Exception Message:
MissingParameter: Parameter 'l1EGCand' not found.
----- End Fatal Exception -------------------------------------------------

And if I also remove L27 and L49-51, nothing will be stored in the hltobject. So I think we should keep it as it is?

I think the issue @missirol is referring to is the observation that hltL1IsoRecoEcalCandidate is not part of any recent menu, so there shouldn't be an object with that label in the Event. So it is weird!
Check whether/how l1EGCand is used.

Yes you are correct. In the modules for HIon HLT menu we should use hltEgammaCandidatesPPOnAA. I looked into the example you provided but not quite sure how to do the customisation for them, could you advice?

It could be something as follows (did not test it).

diff --git a/HLTrigger/Configuration/python/customizeHLTforCMSSW.py b/HLTrigger/Configuration/python/customizeHLTforCMSSW.py index 4f116461823..0b027666ccc 100644 --- a/HLTrigger/Configuration/python/customizeHLTforCMSSW.py +++ b/HLTrigger/Configuration/python/customizeHLTforCMSSW.py @@ -261,6 +261,13 @@ def checkHLTfor43774(process): return process +def customizeHLTfor44746(process): + for modLabel in ['hltDoubleEle10Mass50PPOnAAFilter', 'hltDoubleEle15Mass50PPOnAAFilter']: + if hasattr(process, modLabel): + mod = getattr(process, modLabel) + mod.l1EGCand = cms.InputTag('hltEgammaCandidatesPPOnAA') + return process + # CMSSW version specific customizations def customizeHLTforCMSSW(process, menuType="GRun"): @@ -270,5 +277,6 @@ def customizeHLTforCMSSW(process, menuType="GRun"): # process = customiseFor12718(process) process = checkHLTfor43774(process) + process = customizeHLTfor44746(process) return process

Thanks! I have applied the changes here as well as at PR #44778 in the latest squashed commits.

Sorry there seems to be some merge conflicts. I am trying to deal with them

The conflicts are solved.

missirol · 2024-04-18T18:21:33Z

HLTrigger/Egamma/plugins/HLTEgammaCombMassFilter.cc

+  PrevFilterOutput->getObjects(trigger::TriggerElectron, eleCandsPrev);
+
+  if (!phoCandsPrev.empty()) {
+    for (auto& phoCand : phoCandsPrev) {


Would auto const& work, instead ?

Sure I will fix it, thanks for pointing this out

This is fixed accordingly in the latest commits.

missirol · 2024-04-18T18:23:33Z

HLTrigger/Egamma/plugins/HLTEgammaCombMassFilter.cc

+    }
+  } else if (!clusCandsPrev.empty()) {
+    for (auto& clusCand : clusCandsPrev) {
+      filterproduct.addObject(trigger::TriggerCluster, clusCand);
+    }
+  } else if (!eleCandsPrev.empty()) {
+    for (auto& eleCand : eleCandsPrev) {
+      filterproduct.addObject(trigger::TriggerElectron, eleCand);
+    }
+  }


Normally (but I guess there is no universal rule on this), the trigger objects are added for those candidates which pass some kinematic/trigger selection inside the HLTFilter, e.g. the candidates forming pairs passing the mass cut. Here, it seems that simply every candidate is saved regardless. Is this really intended ?

Sure I will try to fix it, thanks for pointing this out

The problem has been fixed in the latest commits.

cmsbuild · 2024-04-18T20:52:49Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b35ec9/38942/summary.html
COMMIT: 1dab41d
CMSSW: CMSSW_14_0_X_2024-04-18-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/44746/38942/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 23 lines from the logs
Reco comparison results: 63 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3318804
DQMHistoTests: Total failures: 1387
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3317397
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 202 log files, 165 edm output root files, 48 DQM output files
TriggerResults: no differences found

Martin-Grunewald · 2024-05-02T09:51:28Z

code-checks

cmsbuild · 2024-05-02T09:56:58Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44746/40148

This PR adds an extra 20KB 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-44746/40148/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44746/40148/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

missirol · 2024-05-05T12:38:43Z

@pinchunchou

No reply to review comments in over 2 weeks. What is the plan for this PR ?

cmsbuild · 2024-07-03T02:39:45Z

Pull request #44746 was updated. @Martin-Grunewald, @cmsbuild, @mmusich can you please check and sign again.

pinchunchou · 2024-07-03T02:39:57Z

Hmm, looks like merging of #45302 just now leads to a merge conflict in this PR.

@pinchunchou please fix this PR.

fixed

Martin-Grunewald · 2024-07-03T04:18:42Z

please test

cmsbuild · 2024-07-03T09:11:40Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b35ec9/40192/summary.html
COMMIT: 73b2adb
CMSSW: CMSSW_14_0_X_2024-07-02-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/44746/40192/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 89 lines from the logs
Reco comparison results: 39 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3342828
DQMHistoTests: Total failures: 26
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3342782
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 202 log files, 165 edm output root files, 48 DQM output files
TriggerResults: no differences found

Martin-Grunewald · 2024-07-03T09:14:03Z

+1

Martin-Grunewald · 2024-07-03T09:14:09Z

urgent

cmsbuild · 2024-07-03T09:14:27Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_14_0_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_14_1_X is complete. This pull request will now be reviewed by the release team before it's merged. @antoniovilela, @sextonkennedy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

Martin-Grunewald · 2024-07-03T09:14:53Z

@rappoccio @antoniovilela Merge conflicts resolved by author, this PR is now ready to go into 14_0, please merge.

antoniovilela · 2024-07-03T12:39:22Z

+1

antoniovilela · 2024-07-03T12:43:09Z

@rappoccio @antoniovilela Merge conflicts resolved by author, this PR is now ready to go into 14_0, please merge.

@Martin-Grunewald @cms-sw/hlt-l2
Martin, always use @cms-sw/orp-l2 to mention us (which I can more easily filter).
In general, not only this PR, (also) add the hlt-integration label, especially close to a new menu. This will allow me to keep track of what is needed for the HLT menu, as has already been discussed in the past.

cmsbuild added this to the CMSSW_14_0_X milestone Apr 15, 2024

cmsbuild added hlt-pending pending-signatures tests-pending orp-pending labels Apr 15, 2024

mmusich reviewed Apr 16, 2024

View reviewed changes

pinchunchou mentioned this pull request Apr 18, 2024

store the hltobjects for Mass50 triggers #44778

Merged

cmsbuild added bug-fix backport egamma labels Apr 18, 2024

cmsbuild added tests-started and removed tests-pending labels Apr 18, 2024

missirol reviewed Apr 18, 2024

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Apr 18, 2024

pinchunchou force-pushed the from-CMSSW_14_0_0 branch from 13307fa to 73b2adb Compare July 3, 2024 02:39

cmsbuild added hlt-pending pending-signatures tests-pending and removed hlt-approved fully-signed tests-approved labels Jul 3, 2024

cmsbuild added tests-started and removed tests-pending labels Jul 3, 2024

cmsbuild added tests-approved and removed tests-started labels Jul 3, 2024

cmsbuild added hlt-approved fully-signed urgent and removed hlt-pending pending-signatures labels Jul 3, 2024

cmsbuild added orp-approved and removed orp-rejected labels Jul 3, 2024

cmsbuild merged commit 7289f53 into cms-sw:CMSSW_14_0_X Jul 3, 2024
10 checks passed

antoniovilela mentioned this pull request Jul 4, 2024

Build CMSSW_14_0_11 #45376

Closed

		@@ -45,6 +47,35 @@ bool HLTEgammaCombMassFilter::hltFilter(edm::Event& iEvent,
		trigger::TriggerFilterObjectWithRefs& filterproduct) const {
		//right, issue 1, we dont know if this is a TriggerElectron, TriggerPhoton, TriggerCluster (should never be a TriggerCluster btw as that implies the 4-vectors are not stored in AOD)

[14.0.X] store the hltobjects for Mass50 triggers #44746

[14.0.X] store the hltobjects for Mass50 triggers #44746

Conversation

pinchunchou commented Apr 15, 2024

PR description:

cmsbuild commented Apr 15, 2024 • edited Loading

cmsbuild commented Apr 15, 2024 • edited Loading

Martin-Grunewald commented Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmusich commented Apr 17, 2024

pinchunchou commented Apr 17, 2024

cmsbuild commented Apr 18, 2024

cmsbuild commented Apr 18, 2024

pinchunchou commented Apr 18, 2024

mmusich commented Apr 18, 2024

mmusich commented Apr 18, 2024

mmusich commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

missirol Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinchunchou Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Apr 18, 2024

Comparison Summary

Martin-Grunewald commented May 2, 2024

cmsbuild commented May 2, 2024

missirol commented May 5, 2024

cmsbuild commented Jul 3, 2024

pinchunchou commented Jul 3, 2024

Martin-Grunewald commented Jul 3, 2024

cmsbuild commented Jul 3, 2024

Comparison Summary

Martin-Grunewald commented Jul 3, 2024

Martin-Grunewald commented Jul 3, 2024

cmsbuild commented Jul 3, 2024

Martin-Grunewald commented Jul 3, 2024

antoniovilela commented Jul 3, 2024

antoniovilela commented Jul 3, 2024

cmsbuild commented Apr 15, 2024 •

edited

Loading

cmsbuild commented Apr 15, 2024 •

edited

Loading

Martin-Grunewald commented Apr 16, 2024 •

edited

Loading

missirol Jun 18, 2024 •

edited

Loading

pinchunchou Jun 17, 2024 •

edited

Loading