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

HCAL: slimmed collections with non-0 noise flags RecHits for miniAOD #31375

Merged
merged 6 commits into from Sep 12, 2020
Merged

HCAL: slimmed collections with non-0 noise flags RecHits for miniAOD #31375

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dda2c0e
slimmedHcalRecHits for MINIAOD
Sep 3, 2020
30c707b
including HcalSeverityLevelRcd
Sep 3, 2020
c17a680
protection agains HO collection absence
Sep 3, 2020
10d75d3
removig types redefinition
Sep 8, 2020
cb6479c
removig redundant tok association
Sep 8, 2020
0d2f166
fix the HO input treatment
Sep 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions PhysicsTools/PatAlgos/python/slimming/MicroEventContent_cff.py
View file
Open in desktop
Expand Up @@ -32,6 +32,7 @@
'keep EcalRecHitsSorted_reducedEgamma_*_*',
'keep recoGsfTracks_reducedEgamma_*_*',
'keep HBHERecHitsSorted_reducedEgamma_*_*',
'keep *_slimmedHcalRecHits_*_*',
'drop *_*_caloTowers_*',
'drop *_*_pfCandidates_*',
'drop *_*_genJets_*',
Expand Down
2 changes: 2 additions & 0 deletions PhysicsTools/PatAlgos/python/slimming/slimming_cff.py
View file
Open in desktop
Expand Up @@ -27,6 +27,7 @@
from RecoEgamma.EgammaPhotonProducers.reducedEgamma_cfi import *
from RecoLuminosity.LumiProducer.bunchSpacingProducer_cfi import bunchSpacingProducer
from HeavyFlavorAnalysis.Onia2MuMu.OniaPhotonConversionProducer_cfi import PhotonCandidates as oniaPhotonCandidates
from RecoLocalCalo.HcalRecProducers.HcalHitSelection_cfi import *

slimmingTask = cms.Task(
packedPFCandidatesTask,
Expand Down Expand Up @@ -55,6 +56,7 @@
slimmedMETs,
metFilterPathsTask,
reducedEgamma,
slimmedHcalRecHits,
bunchSpacingProducer,
oniaPhotonCandidates
)
Expand Down
8 changes: 8 additions & 0 deletions RecoLocalCalo/HcalRecProducers/python/HcalHitSelection_cfi.py
View file
Open in desktop
@@ -1,5 +1,6 @@
import FWCore.ParameterSet.Config as cms

from RecoLocalCalo.HcalRecAlgos.hcalRecAlgoESProd_cfi import *
reducedHcalRecHits = cms.EDProducer("HcalHitSelection",
hbheTag = cms.InputTag('hbhereco'),
hfTag = cms.InputTag('hfreco'),
Expand All @@ -11,5 +12,12 @@
)
)

slimmedHcalRecHits = reducedHcalRecHits.clone(
hbheTag = cms.InputTag("reducedHcalRecHits","hbhereco"),
hfTag = cms.InputTag("reducedHcalRecHits","hfreco"),
hoTag = cms.InputTag(""),
interestingDetIds = cms.VInputTag()
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're overriding existing PSet items, it would be good to use the type-free syntax: https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideAboutPythonConfigFile#Modifying_Parameters

hbheTag = "reducedHcalRecHits:hbhereco",
...

)

from Configuration.Eras.Modifier_pp_on_AA_2018_cff import pp_on_AA_2018
pp_on_AA_2018.toModify(reducedHcalRecHits.interestingDetIds, func = lambda list: list.remove(cms.InputTag("interestingOotEgammaIsoHCALDetId")) )
4 changes: 3 additions & 1 deletion RecoLocalCalo/HcalRecProducers/src/HcalHitSelection.cc
View file
Open in desktop
Expand Up @@ -169,7 +169,6 @@ void HcalHitSelection::produce(edm::Event& iEvent, const edm::EventSetup& iSetup

iEvent.getByToken(tok_hbhe_, hbhe);
iEvent.getByToken(tok_hf_, hf);
iEvent.getByToken(tok_ho_, ho);

toBeKept.clear();
edm::Handle<DetIdCollection> detId;
Expand All @@ -191,6 +190,9 @@ void HcalHitSelection::produce(edm::Event& iEvent, const edm::EventSetup& iSetup
skim(hf, *hf_out);
iEvent.put(std::move(hf_out), hfTag.label());

if (!iEvent.getByToken(tok_ho_, ho))
return;
iEvent.getByToken(tok_ho_, ho);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder about these lines, as I have not seen such a pattern before. Is it necessary to call getByToken twice with the same arguments? Can the result of the first call not be checked, and passed on?

auto ho_out = std::make_unique<HORecHitCollection>();
skim(ho, *ho_out, hoSeverityLevel);
iEvent.put(std::move(ho_out), hoTag.label());
Copy link
Contributor

@slava77 slava77 Sep 8, 2020 •
edited

Choose a reason for hiding this comment

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

in reco we should still put a declared product into the event (it can be empty) instead of returning.
The if -> return above is more likely an indication of a problem or a bug in configuration.
Why is it done?

Copy link
Author

@abdoulline abdoulline Sep 8, 2020 •
edited

Choose a reason for hiding this comment

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

This new (cloned) mini-AOD slimmedHcalRecHits producer uses an input from reducedHcalRecHits (in AOD), which exists only for HBHE and HF.
So some protection is needed here to re-use the same producer in the absence of HO input (with empty InputTag for HO input in cloned config for simplicity) when there is specifically re-miniAOD from AOD (reducedHcalRecHits for HBHE and HE only), as in wf 136.88811_RunJetHT2018D.
Please, suggest an alternative.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please, suggest an alternative.

configure properly, so that the code does not try to read a collection that is known at configuration step to not exist

Copy link
Author

@abdoulline abdoulline Sep 8, 2020 •
edited

Choose a reason for hiding this comment

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

Slava
(1) it works very well for all wf's as is
(2) there many similar checks in CMSSW
https://cmssdt.cern.ch/lxr/search?%21&_filestring=&_string=%21iEvent.getByToken&_casesensitive=1
(3) your suggestion sounds quite general to me, could you elaborate a bit (given that the config is in this PR), so that I'd put your (simple) suggested update in PR and we move on?

  • I can think of adding a check in the code if InputTag for HO is empty

    cmssw/RecoLocalCalo/HcalRecProducers/src/HcalHitSelection.cc

    Line 123 in cb6479c

    hoTag(iConfig.getParameter<edm::InputTag>("hoTag")),

    then don't do anything for HO (downstream) but it would imply a presence of several switches "if (noHO)" and doesn't look nice...

Copy link
Contributor

Choose a reason for hiding this comment

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

a check for an empty InputTag or a separate flag should be fine.

Copy link
Author

@abdoulline abdoulline Sep 8, 2020 •
edited

Choose a reason for hiding this comment

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

Sorry for confusion... :(
Slava, you're right, while my previous comment was incorrect - there is always HO reducedHcalRecHits in AOD (albeit empty), so no need to add an empty tag to slimmedHcalRecHits producer (clone of reducedHcalRecHits) and to play around it. The simplest option should work. At least It works for 136.88811_RunJetHT2018D (re-minAOD).

Expand Down