Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding a PFJets-Tau overlap removal module for HLT #18997

Merged
merged 9 commits into from
Jun 8, 2017

Conversation

albertdow
Copy link
Contributor

This module is able to remove the overlap between PF jets and taus. The PF jets would be taken from a PFJetCollection and the tau objects come from an HLT filter (so that we can use taus that are ID'ed and isolated). This is necessary for the development of VBF jets + tau HLT paths where we need to cross-clean the PF jets from taus.

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

RecoTauTag/HLTProducers

@Martin-Grunewald, @silviodonato, @cmsbuild, @fwyzard, @davidlange6 can you please review it and eventually sign? Thanks.
@davidlange6 you are the release manager for this.

cms-bot commands are listed here


const edm::EDGetTokenT<trigger::TriggerFilterObjectWithRefs> tauSrc;
const edm::EDGetTokenT<reco::PFJetCollection> PFJetSrc;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a consistent convention for member data - eg, start with lowercase, append an underscore.

}
}
else if(PFJets->size() == 3){
for(unsigned int iTau = 0; iTau < taus.size(); iTau++){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is a special case of 3 needed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi Martin,

the general answer is this module is specific to the VBF seed and its corresponding HLT path (in development). The seed is such that it can have either 2 or 3 jets, and in case it selects 3 jets, we want to clean only those two that make up the larger m_jj.

I let Albert comment on the specific implementation though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
As Riccardo mentioned, there may be 2 or 3 jets of interest in the path. Using the first if statement (with condition PFJets->size() ==2) we cross-clean the 2 jets that make up the highest m_jj (finding these jets is done before by another module within the HLT path). However, if PFJets->size()==3 (else if statement) we again want to cross-clean the 2 jets with highest m_jj but also keep the 3rd jet which will have Pt>90 GeV (this is again decided by a previous module in the HLT path). Thus by adding this special case of PFJets->size()==3 this module can be used for both scenarios.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then the presence of a fourth jet switches from the second back to the first if case,
so any additional jet spoils this. That does not seem robust. Why should the
presence of a fourth jet change what you do with the first three jets?

for(unsigned int iTau = 0; iTau < taus.size(); iTau++){
for(unsigned int iJet = 0; iJet < PFJets->size(); iJet++){
const PFJet & myPFJet = (*PFJets)[iJet];
deltaR = ROOT::Math::VectorUtil::DeltaR((taus[iTau])->p4().Vect(), myPFJet.p4().Vect());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quoting David Lange: please use deltaR2 from DataFormats/Math/interface/deltaR unless some other functionality is intended

for(unsigned int iTau = 0; iTau < taus.size(); iTau++){
for(unsigned int iJet = 0; iJet < PFJets->size()-1; iJet++){
const PFJet & myPFJet = (*PFJets)[iJet];
deltaR = ROOT::Math::VectorUtil::DeltaR((taus[iTau])->p4().Vect(), myPFJet.p4().Vect());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quoting David Lange: please use deltaR2 from DataFormats/Math/interface/deltaR unless some other functionality is intended

@Martin-Grunewald
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 31, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20235/console Started: 2017/05/31 16:08
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20241/console Started: 2017/05/31 16:37

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18997/20241/summary.html

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /build/cmsbld/jenkins-workarea/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-18997/20434.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D10_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D10+RecoFullGlobal_2023D10+HARVESTFullGlobal_2023D10
  • /build/cmsbld/jenkins-workarea/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-18997/23234.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D18_GenSimHLBeamSpotFull14+DigiFull_2023D18+RecoFullGlobal_2023D18+HARVESTFullGlobal_2023D18
  • /build/cmsbld/jenkins-workarea/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-18997/27434.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D17_GenSimHLBeamSpotFull14+DigiFull_2023D17+RecoFullGlobal_2023D17+HARVESTFullGlobal_2023D17

Comparison Summary:

  • You potentially added 31 lines to the logs
  • Reco comparison results: 1709 differences found in the comparisons
  • DQMHistoTests: Total files compared: 23
  • DQMHistoTests: Total histograms compared: 1786072
  • DQMHistoTests: Total failures: 42728
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 1743171
  • DQMHistoTests: Total skipped: 173
  • DQMHistoTests: Total Missing objects: 0
  • Checked 94 log files, 14 edm output root files, 23 DQM output files

@Martin-Grunewald
Copy link
Contributor

Martin-Grunewald commented Jun 1, 2017

@albertdow @vukasinmilosevic

Are you going to need a CaloJets-Tau overlap removal as well?
Then you better rewrite this code and make it a templated class, similar to the matching code #18992 + #19055

@albertdow
Copy link
Contributor Author

albertdow commented Jun 1, 2017

No, a CaloJets-Tau overlap removal module will not be required. Overlap will always be removed between Taus and PFJets.

@Martin-Grunewald
Copy link
Contributor

Martin-Grunewald commented Jun 2, 2017

Please make all the changes discussed in the PR #19068 also here (config parameters, deltaR2, include guard string, etc.)

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 7, 2017

Pull request #18997 was updated. @Martin-Grunewald, @silviodonato, @cmsbuild, @fwyzard, @davidlange6 can you please check and sign again.

@Martin-Grunewald
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 7, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20406/console Started: 2017/06/07 16:58

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 7, 2017

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 7, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 7, 2017

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18997/20406/summary.html

Comparison Summary:

  • You potentially added 170 lines to the logs
  • Reco comparison results: 1720 differences found in the comparisons
  • DQMHistoTests: Total files compared: 23
  • DQMHistoTests: Total histograms compared: 1837615
  • DQMHistoTests: Total failures: 42608
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 1794834
  • DQMHistoTests: Total skipped: 173
  • DQMHistoTests: Total Missing objects: 0
  • Checked 94 log files, 14 edm output root files, 23 DQM output files

@Martin-Grunewald
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2017

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 requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar

@davidlange6
Copy link
Contributor

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants