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.

Sign up for GitHub

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

Jump to bottom

Shower shape variables for HF jets #31271

Merged
merged 9 commits into from
Sep 10, 2020
Merged

Shower shape variables for HF jets #31271

merged 9 commits into from
Sep 10, 2020

Conversation

lathomas
Copy link
Contributor

PR description:

This PR computes a few shower shape related variables for HF jets that are then added as userFloats/userInts to AK4 CHS jets. The variables are also added to NANOAOD.
Such variables turn out to be useful to discriminate physics jets from noise.

The description of the variables and their motivation can be found in this talk:
https://indico.cern.ch/event/944298/#sc-10-4-hf-noise-study-on-aod

PR validation:

runTheMatrix.py -l limited -i all --ibeos gave no error
It was explicitly checked that the variables added to MINIAOD (computed from AOD) and to NANOAOD (computed from MINIAOD) are consistent.
The code quality checks were performed.

if this PR is a backport please specify the original PR and why you need to backport that PR:

Not a backport. A backport to 10_6_X is however expected soon as the goal would ideally be to have this PR integrated for Run 2 UL reMINIAOD.

@cmsbuild cmsbuild added this to the CMSSW_11_2_X milestone Aug 27, 2020
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31271/17952

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

PhysicsTools/NanoAOD
PhysicsTools/PatAlgos
RecoJets/JetProducers

@perrotta, @gouskos, @cmsbuild, @fgolf, @slava77, @jpata, @mariadalfonso, @santocch, @peruzzim can you please review it and eventually sign? Thanks.
@rappoccio, @gouskos, @yslai, @hatakeyamak, @emilbols, @peruzzim, @seemasharmafnal, @mmarionncern, @ahinzmann, @smoortga, @jdolen, @ferencek, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @andrzejnovak, @clelange, @riga, @JyothsnaKomaragiri, @gpetruc, @mariadalfonso this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@mariadalfonso mariadalfonso mentioned this pull request Aug 27, 2020
Closed
55 tasks
@mariadalfonso
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 27, 2020 •
edited
Loading

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+1
Tested at: 908256f
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b9f289/8970/summary.html
CMSSW: CMSSW_11_2_X_2020-08-27-1100
SCRAM_ARCH: slc7_amd64_gcc820

@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-b9f289/8970/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 198 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 2609667
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2609644
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 149 log files, 22 edm output root files, 35 DQM output files

@cmsbuild cmsbuild mentioned this pull request Aug 28, 2020
Merged
slava77
slava77 reviewed Aug 28, 2020
View reviewed changes
PhysicsTools/PatAlgos/python/slimming/miniAOD_tools.py Outdated
Comment on lines 247 to 248
process.patJets.userData.userFloats.src += [ cms.InputTag('HFJetShowerShape:sigmaEtaEta'),cms.InputTag('HFJetShowerShape:sigmaPhiPhi'), ]
process.patJets.userData.userInts.src += [ cms.InputTag('HFJetShowerShape:centralEtaStripSize'),cms.InputTag('HFJetShowerShape:adjacentEtaStripsSize'), ]
Copy link
Contributor

Choose a reason for hiding this comment

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

please drop type specifications for all parameters which already exist.
This is a safer way to protect from parameter name mistakes and will also help in possible parameter migrations.

Suggested change
process.patJets.userData.userFloats.src += [ cms.InputTag('HFJetShowerShape:sigmaEtaEta'),cms.InputTag('HFJetShowerShape:sigmaPhiPhi'), ]
process.patJets.userData.userInts.src += [ cms.InputTag('HFJetShowerShape:centralEtaStripSize'),cms.InputTag('HFJetShowerShape:adjacentEtaStripsSize'), ]
process.patJets.userData.userFloats.src += [ 'HFJetShowerShape:sigmaEtaEta', 'HFJetShowerShape:sigmaPhiPhi', ]
process.patJets.userData.userInts.src += [ 'HFJetShowerShape:centralEtaStripSize', 'HFJetShowerShape:adjacentEtaStripsSize', ]

RecoJets/JetProducers/plugins/HFJetShowerShape.cc Outdated
Comment on lines 55 to 65
edm::EDGetTokenT<edm::View<reco::Jet>> jets_token_;
edm::EDGetTokenT<std::vector<reco::Vertex>> vertices_token_;

//Jet pt/eta thresholds
double jetPtThreshold_, jetEtaThreshold_;
//HF geometry
double hfTowerEtaWidth_, hfTowerPhiWidth_;
//Variables for PU subtraction
double vertexRecoEffcy_, offsetPerPU_, jetReferenceRadius_;
//Pt thresholds for showershape variable calculation
double stripPtThreshold_, widthPtThreshold_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
edm::EDGetTokenT<edm::View<reco::Jet>> jets_token_;
edm::EDGetTokenT<std::vector<reco::Vertex>> vertices_token_;
//Jet pt/eta thresholds
double jetPtThreshold_, jetEtaThreshold_;
//HF geometry
double hfTowerEtaWidth_, hfTowerPhiWidth_;
//Variables for PU subtraction
double vertexRecoEffcy_, offsetPerPU_, jetReferenceRadius_;
//Pt thresholds for showershape variable calculation
double stripPtThreshold_, widthPtThreshold_;
const edm::EDGetTokenT<edm::View<reco::Jet>> jets_token_;
const edm::EDGetTokenT<std::vector<reco::Vertex>> vertices_token_;
//Jet pt/eta thresholds
const double jetPtThreshold_, jetEtaThreshold_;
//HF geometry
const double hfTowerEtaWidth_, hfTowerPhiWidth_;
//Variables for PU subtraction
const double vertexRecoEffcy_, offsetPerPU_, jetReferenceRadius_;
//Pt thresholds for showershape variable calculation
const double stripPtThreshold_, widthPtThreshold_;

all of this is known at configuration time and does not need to change

RecoJets/JetProducers/plugins/HFJetShowerShape.cc Outdated
Comment on lines 80 to 90
jets_token_ = consumes<edm::View<reco::Jet>>(iConfig.getParameter<edm::InputTag>("theJets"));
vertices_token_ = consumes<std::vector<reco::Vertex>>(iConfig.getParameter<edm::InputTag>("theVertices"));
jetPtThreshold_ = iConfig.getParameter<double>("jetPtThreshold");
jetEtaThreshold_ = iConfig.getParameter<double>("jetEtaThreshold");
hfTowerEtaWidth_ = iConfig.getParameter<double>("hfTowerEtaWidth");
hfTowerPhiWidth_ = iConfig.getParameter<double>("hfTowerPhiWidth");
vertexRecoEffcy_ = iConfig.getParameter<double>("vertexRecoEffcy");
offsetPerPU_ = iConfig.getParameter<double>("offsetPerPU");
jetReferenceRadius_ = iConfig.getParameter<double>("jetReferenceRadius");
stripPtThreshold_ = iConfig.getParameter<double>("stripPtThreshold");
widthPtThreshold_ = iConfig.getParameter<double>("widthPtThreshold");
Copy link
Contributor

Choose a reason for hiding this comment

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

move this part to the initializer list (a comma-separated list after iConfig) : )

RecoJets/JetProducers/plugins/HFJetShowerShape.cc Outdated
Comment on lines 98 to 101
HFJetShowerShape::~HFJetShowerShape() {
// do anything here that needs to be done at destruction time
// (e.g. close files, deallocate resources etc.)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this and its declaration; overload only when needed

RecoJets/JetProducers/plugins/HFJetShowerShape.cc Outdated
Comment on lines 112 to 113
edm::Handle<edm::View<reco::Jet>> theJets;
iEvent.getByToken(jets_token_, theJets);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
edm::Handle<edm::View<reco::Jet>> theJets;
iEvent.getByToken(jets_token_, theJets);
auto theJets = iEvent.getHandle(jets_token_);

is a more compact syntax

RecoJets/JetProducers/plugins/HFJetShowerShape.cc Outdated
std::vector<int>* v_size_AdjacentEtaStrips = new std::vector<int>;

//Et offset for HF PF candidates
double PUoffset = offsetPerPU_ / (TMath::Pi() * jetReferenceRadius_ * jetReferenceRadius_) * nPV / vertexRecoEffcy_ *
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
double PUoffset = offsetPerPU_ / (TMath::Pi() * jetReferenceRadius_ * jetReferenceRadius_) * nPV / vertexRecoEffcy_ *
double puOffset = offsetPerPU_ / (M_PI * jetReferenceRadius_ * jetReferenceRadius_) * nPV / vertexRecoEffcy_ *
  • pi is known from elsewhere (and remove TMath include)
  • variable names should not be capitalized

RecoJets/JetProducers/plugins/HFJetShowerShape.cc Outdated
double PUoffset = offsetPerPU_ / (TMath::Pi() * jetReferenceRadius_ * jetReferenceRadius_) * nPV / vertexRecoEffcy_ *
(hfTowerEtaWidth_ * hfTowerPhiWidth_);

for (auto jet = theJets->begin(); jet != theJets->end(); ++jet) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for (auto jet = theJets->begin(); jet != theJets->end(); ++jet) {
for (auto const& jet : *theJets ) {

RecoJets/JetProducers/plugins/HFJetShowerShape.cc Outdated
double eta_jet = jet->eta();

//If central or low pt jets, fill with dummy variables
if (pt_jet <= jetPtThreshold_ || fabs(eta_jet) <= jetEtaThreshold_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (pt_jet <= jetPtThreshold_ || fabs(eta_jet) <= jetEtaThreshold_) {
if (pt_jet <= jetPtThreshold_ || std::abs(eta_jet) <= jetEtaThreshold_) {

std::abs is preferred

RecoJets/JetProducers/plugins/HFJetShowerShape.cc Outdated
Comment on lines 145 to 146
reco::CandidatePtr pfJetConstituent = jet->sourceCandidatePtr(i);
const reco::Candidate* icand = pfJetConstituent.get();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
reco::CandidatePtr pfJetConstituent = jet->sourceCandidatePtr(i);
const reco::Candidate* icand = pfJetConstituent.get();
const reco::Candidate* icand = jet->sourceCandidatePtr(i).get();

just a bit less verbose

RecoJets/JetProducers/python/HFJetShowerShape_cfi.py Outdated
widthPtThreshold = cms.double(3)
)

HFJetShowerShapeTask = cms.Task(HFJetShowerShape)
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for a task for a single module

this file can be removed, simply use RecoJets.JetProducers.hfJetShowerShape_cfi already created by the fillDescriptions.

producer names should also start from lower case (more common style for products in CMSSW reco)

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 9, 2020

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 136 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 2609667
  • DQMHistoTests: Total failures: 5
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2609639
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 2.267 KiB( 34 files compared)
  • DQMHistoSizes: changed ( 10224.0 ): -0.004 KiB MessageLogger/Warnings
  • DQMHistoSizes: changed ( 1325.7 ): 2.271 KiB Physics/NanoAODDQM
  • Checked 149 log files, 22 edm output root files, 35 DQM output files

@slava77
Copy link
Contributor

slava77 commented Sep 9, 2020

+1

for #31271 efc2ace

  • the changes since the last reco signoff are outside of reco (in PhysicsTools/NanoAOD/python/nanoDQM_cfi.py)
  • the plots show up as expected
    https://tinyurl.com/y6j9a8ws

Screen Shot 2020-09-09 at 4 52 24 PM

@santocch
Copy link

+1

@lathomas lathomas mentioned this pull request Sep 10, 2020
Merged
@mariadalfonso
Copy link
Contributor

+xpog
new HF anomalous signal added in miniAOD and nano
re-runned in previous miniAOD

@cmsbuild
Copy link
Contributor

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

@silviodonato
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit d8780f6 into cms-sw:master Sep 10, 2020
@Dr15Jones
Copy link
Contributor

This pull request is causing failures in the following workflows: 10224.15, 11024.15, and 25202.15.

@slava77
Copy link
Contributor

slava77 commented Sep 11, 2020

This pull request is causing failures in the following workflows: 10224.15, 11024.15, and 25202.15.

the issues are apparently related to PhysicsTools/NanoAOD/custom_jme_cff.PrepJMECustomNanoAOD_MC

  [2] Calling method for module SimpleCandidateFlatTableProducer/'jetTable'
Exception Message:
Requested UserInt hfJetShowerShape:adjacentEtaStripsSize is not available!

https://cmssdt.cern.ch/SDT/cgi-bin/logreader/slc7_amd64_gcc820/CMSSW_11_2_X_2020-09-10-2300/pyRelValMatrixLogs/run/10224.15_TTbar_13+2017PU_JMENano+TTbar_13TeV_TuneCUETP8M1_GenSimINPUT+DigiPU+RecoPU+HARVESTPU+Nano/step5_TTbar_13+2017PU_JMENano+TTbar_13TeV_TuneCUETP8M1_GenSimINPUT+DigiPU+RecoPU+HARVESTPU+Nano.log#/232

This will delay the 106X backport

@slava77
Copy link
Contributor

slava77 commented Sep 11, 2020

@lathomas
are you available to take a look and possibly fix it today or by Monday?

@slava77
Copy link
Contributor

slava77 commented Sep 11, 2020

This will delay the 106X backport

uhm, apparently it doesn't have to, at least for the IB integrity; I did not find the JME nano setup in 10_6_X in the matrix.

@lathomas
Copy link
Contributor Author

@lathomas
are you available to take a look and possibly fix it today or by Monday?

Yes I'm looking into it... It looks like it comes from the fact that the producer is only rerun for specific era modifiers...
It seems to work after adding run2_jme_2017 run2_jme_2016 to
https://github.com/cms-sw/cmssw/blob/master/PhysicsTools/NanoAOD/python/jets_cff.py#L697

This was referenced Sep 12, 2020
Merged
Closed
@lathomas lathomas mentioned this pull request Jun 2, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@perrotta perrotta perrotta left review comments

@slava77 slava77 slava77 left review comments

@mariadalfonso mariadalfonso mariadalfonso left review comments

Assignees
No one assigned
Labels
analysis-approved code-checks-approved comparison-available fully-signed orp-approved reconstruction-approved tests-approved xpog-approved
Projects
None yet
Milestone
CMSSW_11_2_X
Development

Successfully merging this pull request may close these issues.
8 participants
@lathomas @cmsbuild @mariadalfonso @slava77 @santocch @silviodonato @Dr15Jones @perrotta