Add HLT filter to select online pf taus based on their impact parameter #36666

sarafiorendi · 2022-01-10T18:29:20Z

PR description:

A new HLTFilter is added in order to be able to apply cuts on the PF tau impact parameters values as calculated by the PFTauTransverseImpactParameter class .
The purpose is to use this filter in an HLT path for Run3 targeting displaced taus.

cmsbuild · 2022-01-10T18:37:31Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36666/27707

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

missirol

Here's the first round of comments: mostly small technical changes, plus some questions.

"Code-checks" failed, and this needs to be fixed. Please remember to run scram b code-checks ; scram b code-format, and commit the suggested changes, before pushing updates to the branch.

HLTrigger/btau/plugins/HLTTauDxyFilter.h

missirol · 2022-01-10T18:38:13Z

HLTrigger/btau/plugins/HLTTauDxyFilter.h

+  edm::InputTag m_Taus;  // module label of input TauCollection
+  edm::EDGetTokenT<std::vector<reco::PFTau>> m_TausToken;
+  edm::InputTag m_TausIP;  // module label of input TauIPCollection
+  edm::EDGetTokenT<edm::AssociationVector<reco::PFTauRefProd,std::vector<reco::PFTauTransverseImpactParameterRef>>> m_TausIPToken;
+
+  double m_MinDxy, m_MaxDxy;  // dxy cuts applied to each tau
+  int m_MinTaus;              // min. number of taus required to pass the cuts
+  int m_TriggerType;


Suggested change

edm::InputTag m_Taus; // module label of input TauCollection

edm::EDGetTokenT<std::vector<reco::PFTau>> m_TausToken;

edm::InputTag m_TausIP; // module label of input TauIPCollection

edm::EDGetTokenT<edm::AssociationVector<reco::PFTauRefProd,std::vector<reco::PFTauTransverseImpactParameterRef>>> m_TausIPToken;

double m_MinDxy, m_MaxDxy; // dxy cuts applied to each tau

int m_MinTaus; // min. number of taus required to pass the cuts

int m_TriggerType;

const edm::InputTag m_TausInputTag; // module label of input TauCollection

const edm::EDGetTokenT<reco::PFTauCollection> m_TausToken;

const edm::EDGetTokenT<edm::AssociationVector<reco::PFTauRefProd,std::vector<reco::PFTauTransverseImpactParameterRef>>> m_TausIPToken;

const double m_MinDxy, m_MaxDxy; // dxy cuts applied to each tau

const int m_MinTaus; // min. number of taus required to pass the cuts

const int m_TriggerType;

missirol · 2022-01-10T18:45:15Z

HLTrigger/btau/plugins/modules.cc

+#include "HLTTauDxyFilter.h"
+DEFINE_FWK_MODULE(HLTTauDxyFilter);
+


Suggested change

#include "HLTTauDxyFilter.h"

DEFINE_FWK_MODULE(HLTTauDxyFilter);

If the header file is not necessary (for example, like for a non-templated plugin like this one), the preferred convention in CMSSW is to have only one single file ([FilterName].cc), which contains both the definition (the .h part), and the implementation part (the .cc part), plus the DEFINE_FWK_MODULE call. It's true that this convention is not followed for all the filters in this package, but I would prefer to stick to the current convention.

missirol · 2022-01-10T18:47:24Z

HLTrigger/btau/plugins/HLTTauDxyFilter.h

+#include "FWCore/Framework/interface/Frameworkfwd.h"
+#include "FWCore/Utilities/interface/InputTag.h"
+#include "HLTrigger/HLTcore/interface/HLTFilter.h"
+#include "DataFormats/TauReco/interface/PFTau.h"


Suggested change

#include "DataFormats/TauReco/interface/PFTau.h"

#include "DataFormats/TauReco/interface/PFTau.h"

#include "DataFormats/TauReco/interface/PFTauFwd.h"

missirol · 2022-01-10T18:49:36Z

HLTrigger/btau/plugins/HLTTauDxyFilter.cc

+  using namespace edm;
+


Suggested change

using namespace edm;

missirol · 2022-01-10T19:07:31Z

HLTrigger/btau/plugins/HLTTauDxyFilter.cc

+#include <string>
+
+#include "FWCore/MessageLogger/interface/MessageLogger.h"
+#include "DataFormats/Common/interface/Handle.h"


Suggested change

#include "DataFormats/Common/interface/Handle.h"

missirol · 2022-01-10T19:08:07Z

HLTrigger/btau/plugins/HLTTauDxyFilter.cc

+#include "HLTrigger/HLTcore/interface/defaultModuleLabel.h"
+
+
+//
+// constructors and destructor
+//


Suggested change

#include "HLTrigger/HLTcore/interface/defaultModuleLabel.h"

//

// constructors and destructor

//

missirol · 2022-01-10T19:11:11Z

HLTrigger/btau/plugins/HLTTauDxyFilter.cc

+}
+
+
+// ------------ method called to produce the data  ------------


Suggested change

// ------------ method called to produce the data ------------

missirol · 2022-01-10T19:12:53Z

HLTrigger/btau/plugins/HLTTauDxyFilter.h

+namespace edm {
+  class ConfigurationDescriptions;
+}
+
+//
+// class declaration
+//
+


Suggested change

namespace edm {

class ConfigurationDescriptions;

}

//

// class declaration

//

missirol · 2022-01-10T19:18:35Z

HLTrigger/btau/plugins/HLTTauDxyFilter.cc

+      m_Taus(config.getParameter<edm::InputTag>("Taus")),
+      m_TausIP(config.getParameter<edm::InputTag>("TausIP")),
+      m_MinDxy(config.getParameter<double>("MinDxy")),
+      m_MaxDxy(config.getParameter<double>("MaxDxy")),
+      m_MinTaus(config.getParameter<int>("MinN")),
+      m_TriggerType(config.getParameter<int>("TriggerType")) {
+  m_TausToken = consumes<std::vector<reco::PFTau>>(m_Taus), 
+  m_TausIPToken = consumes<edm::AssociationVector<reco::PFTauRefProd,std::vector<reco::PFTauTransverseImpactParameterRef>>>(m_TausIP),


Suggested change

m_Taus(config.getParameter<edm::InputTag>("Taus")),

m_TausIP(config.getParameter<edm::InputTag>("TausIP")),

m_MinDxy(config.getParameter<double>("MinDxy")),

m_MaxDxy(config.getParameter<double>("MaxDxy")),

m_MinTaus(config.getParameter<int>("MinN")),

m_TriggerType(config.getParameter<int>("TriggerType")) {

m_TausToken = consumes<std::vector<reco::PFTau>>(m_Taus),

m_TausIPToken = consumes<edm::AssociationVector<reco::PFTauRefProd,std::vector<reco::PFTauTransverseImpactParameterRef>>>(m_TausIP),

m_TausInputTag(config.getParameter<edm::InputTag>("Taus")),

m_TausToken(consumes(m_TausInputTag)),

m_TausIPToken(consumes(config.getParameter<edm::InputTag>("TausIP"))),

m_MinDxy(config.getParameter<double>("MinDxy")),

m_MaxDxy(config.getParameter<double>("MaxDxy")),

m_MinTaus(config.getParameter<int>("MinN")),

m_TriggerType(config.getParameter<int>("TriggerType")) {

cmsbuild · 2022-01-10T20:01:31Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36666/27709

This PR adds an extra 16KB to repository

cmsbuild · 2022-01-10T20:01:52Z

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

It involves the following packages:

HLTrigger/btau (hlt)

@cmsbuild, @missirol, @Martin-Grunewald can you please review it and eventually sign? Thanks.
@AlexDeMoor, @emilbols, @JyothsnaKomaragiri, @silviodonato, @Martin-Grunewald, @missirol, @andrzejnovak, @demuller 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

cmsbuild · 2022-01-10T21:27:33Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36666/27711

ERROR: Build errors found during clang-tidy run.

HLTrigger/btau/plugins/HLTTauDxyFilter.cc:37:18: error: definition of implicitly declared destructor [clang-diagnostic-error]
HLTTauDxyFilter::~HLTTauDxyFilter() = default;
                 ^
Suppressed 1456 warnings (1452 in non-user code, 3 NOLINT, 1 with check filters).
--
gmake: *** [config/SCRAM/GMake/Makefile.coderules:129: code-checks] Error 2
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

sarafiorendi · 2022-01-10T22:40:09Z

Hi @missirol,
thanks for the review. I applied almost all of your suggestions, but I still need to answer about if a check for a valid TIP reference is needed.

cmsbuild · 2022-01-10T22:44:02Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36666/27712

This PR adds an extra 16KB to repository
Found files with invalid states:
- HLTrigger/btau/plugins/HLTTauDxyFilter.h:
  - Added: c82e939
  - Modified: d4bae1f, 434f3fe
  - Deleted: 329e5d7

cmsbuild · 2022-01-10T22:44:28Z

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

missirol

Thanks @sarafiorendi , I will wait for you to check that.

One more suggestion below to adjust the #include statements.

missirol · 2022-01-10T23:00:09Z

HLTrigger/btau/plugins/HLTTauDxyFilter.cc

+#include "FWCore/MessageLogger/interface/MessageLogger.h"
+#include "DataFormats/Common/interface/RefToBase.h"
+#include "DataFormats/HLTReco/interface/TriggerFilterObjectWithRefs.h"
+#include "FWCore/ParameterSet/interface/ConfigurationDescriptions.h"
+#include "FWCore/ParameterSet/interface/ParameterSetDescription.h"


Suggested change

#include "FWCore/MessageLogger/interface/MessageLogger.h"

#include "DataFormats/Common/interface/RefToBase.h"

#include "DataFormats/HLTReco/interface/TriggerFilterObjectWithRefs.h"

#include "FWCore/ParameterSet/interface/ConfigurationDescriptions.h"

#include "FWCore/ParameterSet/interface/ParameterSetDescription.h"

Please move these header files upstairs with the other ones. I'm not sure RefToBase.h is needed, I would try to remove it.

missirol · 2022-01-10T23:01:46Z

HLTrigger/btau/plugins/HLTTauDxyFilter.cc

+  return accept;
+}
+
+#include "FWCore/Framework/interface/MakerMacros.h"


Suggested change

#include "FWCore/Framework/interface/MakerMacros.h"

This one can also go upstairs.

missirol · 2022-01-10T23:04:59Z

HLTrigger/btau/plugins/HLTTauDxyFilter.cc

+#include "FWCore/Framework/interface/Frameworkfwd.h"
+#include "FWCore/Utilities/interface/InputTag.h"
+#include "HLTrigger/HLTcore/interface/HLTFilter.h"
+#include "DataFormats/TauReco/interface/PFTau.h"
+#include "DataFormats/TauReco/interface/PFTauFwd.h"
+#include "DataFormats/TauReco/interface/PFTauTransverseImpactParameterAssociation.h"
+
+#include <vector>
+#include <string>


Suggested change

#include "FWCore/Framework/interface/Frameworkfwd.h"

#include "FWCore/Utilities/interface/InputTag.h"

#include "HLTrigger/HLTcore/interface/HLTFilter.h"

#include "DataFormats/TauReco/interface/PFTau.h"

#include "DataFormats/TauReco/interface/PFTauFwd.h"

#include "DataFormats/TauReco/interface/PFTauTransverseImpactParameterAssociation.h"

#include <vector>

#include <string>

#include "FWCore/Framework/interface/Frameworkfwd.h"

#include "FWCore/Framework/interface/MakerMacros.h"

#include "FWCore/MessageLogger/interface/MessageLogger.h"

#include "FWCore/ParameterSet/interface/ConfigurationDescriptions.h"

#include "FWCore/ParameterSet/interface/ParameterSetDescription.h"

#include "FWCore/Utilities/interface/InputTag.h"

#include "FWCore/Utilities/interface/EDGetToken.h"

#include "DataFormats/TauReco/interface/PFTau.h"

#include "DataFormats/TauReco/interface/PFTauFwd.h"

#include "DataFormats/TauReco/interface/PFTauTransverseImpactParameterAssociation.h"

#include "HLTrigger/HLTcore/interface/HLTFilter.h"

#include <vector>

sarafiorendi · 2022-01-11T09:24:25Z

Hi Marino,
checks for a valid reference are performed by the PFTauTransverseImpactParameters class here [1] and [2] so I think a further check in this new filter is not needed.

[1]
https://github.com/cms-sw/cmssw/blob/master/RecoTauTag/RecoTau/plugins/PFTauTransverseImpactParameters.cc#L84
[2]
https://github.com/cms-sw/cmssw/blob/master/RecoTauTag/RecoTau/plugins/PFTauTransverseImpactParameters.cc#L135

cmsbuild · 2022-01-14T12:37:52Z

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

missirol

Last set of comments. Some were missed in the previous round, and are repeated below.

Optional: consider squashing again into 1 commit before the final push.

missirol · 2022-01-14T12:46:17Z

RecoTauTag/HLTProducers/src/HLTTauIPFilter.cc

@@ -0,0 +1,94 @@
+/** \class HLTTauIPFilter


Should have been clearer: no problem with "IP", but I would still prefer "PFTau".
Could we please use HLTPFTauIPFilter since the filter deals with "PFTau"s? (atm, we have other filters/producers in the menu named HLTPFTau*, but I couldn't find any called HLTTau*)

missirol · 2022-01-14T12:46:54Z

RecoTauTag/HLTProducers/src/HLTTauIPFilter.cc

+#include "DataFormats/TauReco/interface/PFTauTransverseImpactParameterAssociation.h"
+#include "HLTrigger/HLTcore/interface/HLTFilter.h"
+#include "CommonTools/Utils/interface/StringCutObjectSelector.h"
+#include <vector>


From previous round of comments.

missirol · 2022-01-14T12:47:17Z

RecoTauTag/HLTProducers/src/HLTTauIPFilter.cc

+  std::string m_tauTIPSelectorString;
+  StringCutObjectSelector<reco::PFTauTransverseImpactParameter> m_tauTIPSelector;


Suggested change

std::string m_tauTIPSelectorString;

StringCutObjectSelector<reco::PFTauTransverseImpactParameter> m_tauTIPSelector;

const std::string m_tauTIPSelectorString;

const StringCutObjectSelector<reco::PFTauTransverseImpactParameter> m_tauTIPSelector;

From previous round of comments.

missirol · 2022-01-14T12:48:33Z

RecoTauTag/HLTProducers/src/HLTTauIPFilter.cc

+  for (reco::PFTauCollection::size_type iPFTau = 0; iPFTau < h_Taus->size(); iPFTau++) {
+    reco::PFTauRef tauref(h_Taus, iPFTau);
+
+    if ((m_tauTIPSelector)(*TausTIP[tauref])) {


Suggested change

if ((m_tauTIPSelector)(*TausTIP[tauref])) {

if (m_tauTIPSelector(*TausTIP[tauref])) {

Parentheses not needed.

more generic name after change to cut string apply code format implement latest suggestions rename pftau, add const

cmsbuild · 2022-01-14T13:18:20Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36666/27788

This PR adds an extra 16KB to repository
Found files with invalid states:
- RecoTauTag/HLTProducers/src/HLTTauDxyFilter.cc:
  - Added: 91cd418
  - Deleted: fd3fedb

cmsbuild · 2022-01-14T13:18:45Z

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

missirol · 2022-01-14T13:32:49Z

please test

cmsbuild · 2022-01-14T17:55:17Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ab50aa/21724/summary.html
COMMIT: fd3fedb
CMSSW: CMSSW_12_3_X_2022-01-14-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36666/21724/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: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 43
DQMHistoTests: Total histograms compared: 3461659
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3461637
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 42 files compared)
Checked 181 log files, 42 edm output root files, 43 DQM output files
TriggerResults: no differences found

missirol · 2022-01-14T20:23:56Z

+hlt

new HLTFilter to select PFTaus based on their IP properties
presently not used in any validation wf (nor any HLT menu)

cmsbuild · 2022-01-14T20:24:19Z

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)

missirol · 2022-01-14T20:25:21Z

@sarafiorendi , it would be great if you could update the PR title and description to reflect the latest changes to the plugin (dxy -> IP properties).

qliphy · 2022-01-17T03:24:50Z

@sarafiorendi , it would be great if you could update the PR title and description to reflect the latest changes to the plugin (dxy -> IP properties).

ping @sarafiorendi

qliphy · 2022-01-18T00:38:46Z

+1

cmsbuild added this to the CMSSW_12_3_X milestone Jan 10, 2022

cmsbuild added code-checks-pending hlt-pending orp-pending pending-signatures tests-pending labels Jan 10, 2022

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jan 10, 2022

missirol reviewed Jan 10, 2022

View reviewed changes

cmsbuild added code-checks-pending and removed code-checks-rejected labels Jan 10, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 10, 2022

cmsbuild added code-checks-pending and removed code-checks-approved labels Jan 10, 2022

cmsbuild added code-checks-rejected code-checks-pending and removed code-checks-pending code-checks-rejected labels Jan 10, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 10, 2022

missirol reviewed Jan 10, 2022

View reviewed changes

missirol reviewed Jan 14, 2022

View reviewed changes

apply latest suggestions,move to cut string obj sel

fd3fedb

more generic name after change to cut string apply code format implement latest suggestions rename pftau, add const

sarafiorendi force-pushed the tauhlt_dxyfilter branch from 94422ce to fd3fedb Compare January 14, 2022 13:11

cmsbuild added code-checks-pending and removed code-checks-approved labels Jan 14, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 14, 2022

cmsbuild added tests-started and removed tests-pending labels Jan 14, 2022

cmsbuild added tests-approved and removed tests-started labels Jan 14, 2022

cmsbuild added fully-signed hlt-approved and removed hlt-pending pending-signatures labels Jan 14, 2022

sarafiorendi changed the title ~~Add HLT filter to select online taus based on their dxy~~ Add HLT filter to select online pf taus based on their impact parameter Jan 17, 2022

cmsbuild added orp-approved and removed orp-pending labels Jan 18, 2022

cmsbuild merged commit d35c4fe into cms-sw:master Jan 18, 2022

missirol mentioned this pull request Feb 7, 2022

HLT Triggers based on ParticleNet for Run3 #36862

Merged

		#include "HLTTauDxyFilter.h"
		DEFINE_FWK_MODULE(HLTTauDxyFilter);

	#include "DataFormats/TauReco/interface/PFTau.h"
	#include "DataFormats/TauReco/interface/PFTau.h"
	#include "DataFormats/TauReco/interface/PFTauFwd.h"

		}


		// ------------ method called to produce the data ------------

		std::string m_tauTIPSelectorString;
		StringCutObjectSelector<reco::PFTauTransverseImpactParameter> m_tauTIPSelector;

	if ((m_tauTIPSelector)(*TausTIP[tauref])) {
	if (m_tauTIPSelector(*TausTIP[tauref])) {

Add HLT filter to select online pf taus based on their impact parameter #36666

Add HLT filter to select online pf taus based on their impact parameter #36666

Conversation

sarafiorendi commented Jan 10, 2022 • edited

PR description:

cmsbuild commented Jan 10, 2022

missirol left a 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

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

cmsbuild commented Jan 10, 2022

cmsbuild commented Jan 10, 2022

cmsbuild commented Jan 10, 2022

sarafiorendi commented Jan 10, 2022

cmsbuild commented Jan 10, 2022

cmsbuild commented Jan 10, 2022

missirol left a 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

sarafiorendi commented Jan 11, 2022

cmsbuild commented Jan 14, 2022

missirol left a 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

cmsbuild commented Jan 14, 2022

cmsbuild commented Jan 14, 2022

missirol commented Jan 14, 2022

cmsbuild commented Jan 14, 2022

Comparison Summary

missirol commented Jan 14, 2022

cmsbuild commented Jan 14, 2022

missirol commented Jan 14, 2022

qliphy commented Jan 17, 2022

qliphy commented Jan 18, 2022

sarafiorendi commented Jan 10, 2022 •

edited