Producer and filter for jet timing trigger #35724

mcitron · 2021-10-19T03:30:05Z

PR description:

This PR adds the producer and filter needed to define an HLT path for delayed jets. The necessary information is added via value maps.

TSG presentation: https://indico.cern.ch/event/1080507/#39-delayed-jet-hlt-trigger

PR validation:

runTheMatrix.py -l limited -i all --ibeos
32 0 0 0 0 0 0 0 0 tests passed, 8 32 0 0 0 0 0 0 0 failed

cmsbuild · 2021-10-19T03:40:23Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35724/26048

This PR adds an extra 16KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-checks:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35724/26048/code-checks.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35724/26048/code-checks.patch | patch -p1
You can also run scram build code-checks to apply code checks directly
code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35724/26048/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35724/26048/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2021-10-19T03:59:05Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35724/26049

This PR adds an extra 20KB to repository

cmsbuild · 2021-10-19T03:59:28Z

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

It involves the following packages:

HLTrigger/JetMET (hlt)

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

cms-bot commands are listed here

missirol · 2021-10-19T05:38:57Z

Hi @mcitron , I'll add a review on the code in the next days. In the meantime, I add here two general comments:

please merge the header (.h) and source (.cc) files, and move the two new files under plugins/, to comply with the coding rules (sections 6.6 and 6.7);
how can these new plugins be tested? Running the standard PR tests won't exercise these new classes. Could you please provide a simple cfg file under test/ that allows to test this new producer+filter?

cmsbuild · 2021-10-20T00:23:44Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35724/26079

This PR adds an extra 20KB to repository
Found files with invalid states:
- HLTrigger/JetMET/interface/HLTCaloJetTimingFilter.h:
  - Added: 53a03bf
  - Modified: 72eb2e1
  - Deleted: 9f7f0d7
- HLTrigger/JetMET/interface/HLTCaloJetTimingProducer.h:
  - Added: 53a03bf
  - Modified: 72eb2e1
  - Deleted: 9f7f0d7
- HLTrigger/JetMET/src/HLTCaloJetTimingProducer.cc:
  - Added: 53a03bf
  - Modified: 72eb2e1
  - Deleted: 9f7f0d7
- HLTrigger/JetMET/src/HLTCaloJetTimingFilter.cc:
  - Added: 53a03bf
  - Modified: 72eb2e1
  - Deleted: 9f7f0d7

cmsbuild · 2021-10-20T00:24:07Z

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

mcitron · 2021-10-20T00:24:50Z

@missirol thanks for the comments. The header and source files have been merged into new files in the plugins directory and a test config has been added. This produces an output that contains an edm::TriggerResults object which accepts 73/722 events of the input file.

Martin-Grunewald · 2021-10-20T08:47:57Z

HLTrigger/JetMET/plugins/HLTCaloJetTimingFilter.cc

+  edm::EDGetTokenT<reco::CaloJetCollection> jetInputToken;
+  edm::EDGetTokenT<edm::ValueMap<float>> jetTimesInputToken;
+  edm::EDGetTokenT<edm::ValueMap<unsigned int>> jetCellsForTimingInputToken;
+  edm::EDGetTokenT<edm::ValueMap<float>> jetEcalEtForTimingInputToken;


All class variables could probably be made const.

Martin-Grunewald · 2021-10-20T08:49:31Z

HLTrigger/JetMET/plugins/HLTCaloJetTimingFilter.cc

+// class declaration
+//
+class HLTCaloJetTimingFilter : public HLTFilter {
+public:


This filter does not seem to make use of HLTFilter features (eg, filling the filterproduct), so should then just be an EDFilter.

Just noting that this point is still open. For examples on the use of HLTFilter and filterproduct, see the many examples in HLTrigger/HLTfilters/.

thanks for the comment - I have updated the code to use the filterproduct

HLTrigger/JetMET/plugins/HLTCaloJetTimingFilter.cc

Martin-Grunewald · 2021-10-20T08:50:43Z

HLTrigger/JetMET/plugins/HLTCaloJetTimingProducer.cc

+  edm::EDGetTokenT<reco::CaloJetCollection> jetInputToken;
+  edm::EDGetTokenT<edm::SortedCollection<EcalRecHit, edm::StrictWeakOrdering<EcalRecHit>>> ecalRecHitsEBToken;
+  edm::EDGetTokenT<edm::SortedCollection<EcalRecHit, edm::StrictWeakOrdering<EcalRecHit>>> ecalRecHitsEEToken;
+};


As above, member variables look like they could be const.

HLTrigger/JetMET/plugins/HLTCaloJetTimingProducer.cc

missirol

@mcitron, here are the suggested changes. They partly overlap with Martin's. They are largely technical (outputs should be unchanged), but they were done on GH without testing (the code may very well not compile). Please review them carefully, and retest the code.

HLTrigger/JetMET/plugins/HLTCaloJetTimingFilter.cc

HLTrigger/JetMET/plugins/HLTCaloJetTimingProducer.cc

HLTrigger/JetMET/test/hltCaloJetTimingFilter_cfg.py

HLTrigger/JetMET/plugins/HLTCaloJetTimingProducer.cc

Applying all suggested changes for testing/updating as needed Co-authored-by: Marino Missiroli <m.missiroli@cern.ch>

cmsbuild · 2021-10-29T17:09:35Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35724/26317

This PR adds an extra 68KB to repository
Found files with invalid states:
- HLTrigger/JetMET/interface/HLTCaloJetTimingFilter.h:
  - Added: 53a03bf
  - Modified: 72eb2e1
  - Deleted: 9f7f0d7
- HLTrigger/JetMET/interface/HLTCaloJetTimingProducer.h:
  - Added: 53a03bf
  - Modified: 72eb2e1
  - Deleted: 9f7f0d7
- HLTrigger/JetMET/plugins/HLTCaloJetTimingFilter.cc:
  - Added: 9f7f0d7
  - Modified: c75d5ab, 0e2a454, a61f130, 64dc085, 0c751ac, 9f1aa4d, 790f2af
  - Deleted: 8504e36
- HLTrigger/JetMET/plugins/HLTCaloJetTimingProducer.cc:
  - Added: 9f7f0d7
  - Modified: c75d5ab, 0e2a454, 5d65413, a61f130, 0c751ac, b2005d8
  - Deleted: 8504e36
- HLTrigger/JetMET/src/HLTCaloJetTimingProducer.cc:
  - Added: 53a03bf
  - Modified: 72eb2e1
  - Deleted: 9f7f0d7
- HLTrigger/JetMET/src/HLTCaloJetTimingFilter.cc:
  - Added: 53a03bf
  - Modified: 72eb2e1
  - Deleted: 9f7f0d7

cmsbuild · 2021-10-29T17:09:59Z

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

missirol · 2021-10-29T19:03:52Z

Just a note: I remembered from #33204 (comment) that addWithDefaultLabel will also return exclusive cfis for templated classes, using the name of the instantiated class itself (instead of "TemplateClassTemplateType"); in that case, the cfis would have been

hltCaloJetTimingFilter_cfi.py
hltCaloJetTimingProducer_cfi.py
hltpfJetTimingFilter_cfi.py
hltpfJetTimingProducer_cfi.py

I don't think I have any further comments; I'll re-trigger the tests (which are anyway not testing this new module beyond compiling).

missirol · 2021-10-29T19:04:31Z

please test with #35890

the other PR is only needed in order to avoid a timeout in wf 138.3

cmsbuild · 2021-10-29T22:15:11Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c4be56/20095/summary.html
COMMIT: 188ab90
CMSSW: CMSSW_12_2_X_2021-10-29-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35724/20095/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: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 2901440
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2901411
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

missirol · 2021-11-03T07:36:01Z

+hlt

adds two plugins targeting HLT, but not currently used in any workflow
(this is why I don't think it's necessary to rerun the tests with a more recent IB)
a standalone config file to test the new modules is added in test/
despite the label, this PR did not really require any externals
(the other PR was needed only in order to avoid a timeout in a PR test)

cmsbuild · 2021-11-03T07:36:21Z

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

perrotta

Not fundamental comments, but since I already wrote them...

perrotta · 2021-11-03T11:32:07Z

HLTrigger/JetMET/plugins/HLTJetTimingFilter.h

+#include "HLTrigger/HLTcore/interface/HLTFilter.h"
+
+#include "FWCore/Framework/interface/Event.h"
+#include "FWCore/Framework/interface/MakerMacros.h"


If you want to keep separated .h and .cc files, this can be included in the implementation file

perrotta · 2021-11-03T11:32:27Z

HLTrigger/JetMET/plugins/HLTJetTimingProducer.h

+#include "FWCore/Framework/interface/stream/EDProducer.h"
+
+#include "FWCore/Framework/interface/Event.h"
+#include "FWCore/Framework/interface/MakerMacros.h"


If you want to keep separated .h and .cc files, this can be included in the implementation file

perrotta · 2021-11-03T11:41:32Z

+1

added producer and filter for jet timing trigger

53a03bf

cmsbuild added this to the CMSSW_12_1_X milestone Oct 19, 2021

cmsbuild added code-checks-pending hlt-pending orp-pending pending-signatures tests-pending labels Oct 19, 2021

cmsbuild added code-checks-rejected and removed code-checks-pending labels Oct 19, 2021

apply code-format and code-checks changes

72eb2e1

cmsbuild added code-checks-pending and removed code-checks-rejected labels Oct 19, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 19, 2021

mcitron added 2 commits October 19, 2021 17:08

moved calojet timing filter and producer to plugins

9f7f0d7

added test config for hlt calo jet timing

ba46057

cmsbuild added code-checks-pending and removed code-checks-approved labels Oct 20, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 20, 2021

Martin-Grunewald reviewed Oct 20, 2021

View reviewed changes

missirol reviewed Oct 26, 2021

View reviewed changes

Apply suggestions from code review

c75d5ab

Applying all suggested changes for testing/updating as needed Co-authored-by: Marino Missiroli <m.missiroli@cern.ch>

cmsbuild added code-checks-rejected and removed code-checks-pending labels Oct 29, 2021

format

188ab90

cmsbuild added code-checks-pending and removed code-checks-rejected labels Oct 29, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 29, 2021

cmsbuild added requires-external tests-started and removed tests-pending labels Oct 29, 2021

cmsbuild added tests-approved and removed tests-started labels Oct 29, 2021

cmsbuild added fully-signed hlt-approved and removed hlt-pending pending-signatures labels Nov 3, 2021

perrotta reviewed Nov 3, 2021

View reviewed changes

cmsbuild added orp-approved and removed orp-pending labels Nov 3, 2021

cmsbuild merged commit 48d22fd into cms-sw:master Nov 3, 2021

missirol mentioned this pull request Nov 3, 2021

small technical updates to HLTJetTiming{Filter,Producer} #35971

Merged

missirol mentioned this pull request Mar 24, 2023

Add upper cut on jet time to HLTJetTimingFilter #41089

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Producer and filter for jet timing trigger #35724

Producer and filter for jet timing trigger #35724

mcitron commented Oct 19, 2021

cmsbuild commented Oct 19, 2021

cmsbuild commented Oct 19, 2021

cmsbuild commented Oct 19, 2021

missirol commented Oct 19, 2021

cmsbuild commented Oct 20, 2021

cmsbuild commented Oct 20, 2021

mcitron commented Oct 20, 2021

Martin-Grunewald Oct 20, 2021

Martin-Grunewald Oct 20, 2021

missirol Oct 27, 2021

mcitron Oct 27, 2021

Martin-Grunewald Oct 20, 2021

missirol left a comment

cmsbuild commented Oct 29, 2021

cmsbuild commented Oct 29, 2021

missirol commented Oct 29, 2021

missirol commented Oct 29, 2021

cmsbuild commented Oct 29, 2021

missirol commented Nov 3, 2021

cmsbuild commented Nov 3, 2021

perrotta left a comment

perrotta Nov 3, 2021

perrotta Nov 3, 2021

perrotta commented Nov 3, 2021

Producer and filter for jet timing trigger #35724

Producer and filter for jet timing trigger #35724

Conversation

mcitron commented Oct 19, 2021

PR description:

PR validation:

cmsbuild commented Oct 19, 2021

cmsbuild commented Oct 19, 2021

cmsbuild commented Oct 19, 2021

missirol commented Oct 19, 2021

cmsbuild commented Oct 20, 2021

cmsbuild commented Oct 20, 2021

mcitron commented Oct 20, 2021

Martin-Grunewald Oct 20, 2021

Choose a reason for hiding this comment

Martin-Grunewald Oct 20, 2021

Choose a reason for hiding this comment

missirol Oct 27, 2021

Choose a reason for hiding this comment

mcitron Oct 27, 2021

Choose a reason for hiding this comment

Martin-Grunewald Oct 20, 2021

Choose a reason for hiding this comment

missirol left a comment

Choose a reason for hiding this comment

cmsbuild commented Oct 29, 2021

cmsbuild commented Oct 29, 2021

missirol commented Oct 29, 2021

missirol commented Oct 29, 2021

cmsbuild commented Oct 29, 2021

Comparison Summary

missirol commented Nov 3, 2021

cmsbuild commented Nov 3, 2021

perrotta left a comment

Choose a reason for hiding this comment

perrotta Nov 3, 2021

Choose a reason for hiding this comment

perrotta Nov 3, 2021

Choose a reason for hiding this comment

perrotta commented Nov 3, 2021