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 few jet and event variables to prepare for automatic JEC workflows #246

Merged

Conversation

rappoccio
Copy link

To allow the JERC groups to use the NANOAOD, in preparation for automatic JEC workflows, we are adding:

Two variables with precision 6 to the AK4 jet collection (charged hadron fractions from PV and non-PV, with a delta-R definition instead of clustering definition).
Four event-wide floats (pileup density, gen-level pileup density, and two additional rho variables).
This will require the integration of a C++ module into CMSSW, PRs are here:

9.4.x: cms-sw#25225
10.2.x: cms-sw#25226
10.3.x: cms-sw#25224
10.4.x: cms-sw#25223

@rappoccio
Copy link
Author

FYI these were merged last week.

double refpvz = -1000.0;
if ( iEvent.getByToken(genTag_, genParticles) ) {
for ( auto genIt = genParticles->begin(); genIt != genParticles->end(); ++genIt ) {
if ( genIt->statusFlags().isHardProcess() ) {
Copy link

Choose a reason for hiding this comment

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

I tested the code and it seems that none of the packedGenParticles satisfy this condition. What I see is that the variable refpvz will always have a value of -1000, which in turn means that the zbin variable in fillNPUObjectTable() refers to the last element of vz_. Provided that the element beyond vz_ array is 0, as is usually the case in practice (even though it could be any number), the denominator on line 74 is -12 * 20 = -240.
I think that packedGenParticles (nor prunedGenParticles) are not suitable for this logic by construction:

for(unsigned int ic=0, nc = cands->size(); ic < nc; ++ic) {
const reco::GenParticle &cand=(*cands)[ic];
if(cand.status() ==1 && std::abs(cand.y()) < maxRapidity_)
{
// Obtain original gen particle collection reference from input reference and map
edm::Ref<reco::GenParticleCollection> inputRef = edm::Ref<reco::GenParticleCollection>(cands,ic);
edm::Ref<reco::GenParticleCollection> originalRef=reverseMap[inputRef];
edm::Ref<reco::GenParticleCollection> finalPrunedRef=(*asso)[inputRef];
mapping[originalRef.key()]=packed;
packed++;
if(finalPrunedRef.isNonnull()){ //this particle exists also in the final pruned
outPtrP->push_back( pat::PackedGenParticle(cand,finalPrunedRef));
}else{
if(cand.numberOfMothers() > 0) {
edm::Ref<reco::GenParticleCollection> newRef=(*asso)[cand.motherRef(0)];
outPtrP->push_back( pat::PackedGenParticle(cand,newRef));
} else {
outPtrP->push_back( pat::PackedGenParticle(cand,edm::Ref<reco::GenParticleCollection>()));
}
}
}
}

What I suggest is to instead smuggle offlineSlimmedPrimaryVertices into this class, and read refpvz with:
pvTable->addColumnValue<float>("z",(*pvsIn)[0].position().z(),"main primary vertex position z coordinate",nanoaod::FlatTable::FloatColumn,16);

auto bin = std::lower_bound( vz_.begin(), vz_.end()-1, std::abs(zpositions.back()) );
if ( bin != vz_.end() && bin==zbin) gpudensity++;
}
gpudensity/=(20.0*( *(zbin+1) - *(zbin) ) ) ;
Copy link

Choose a reason for hiding this comment

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

This shoukd be:

Suggested change
gpudensity/=(20.0*( *(zbin+1) - *(zbin) ) ) ;
gpudensity/=(20.0*( *(zbin) - *(zbin - 1) ) ) ;

because zbin refers to the value in vz_ array that is greater than or equal to refpvz. In other words, it's the right edge of the bin. Provided that the binning zbins in python config starts from 0 (as you already have), it's safe to decrement the iterator by one step.

@ktht
Copy link

ktht commented Dec 12, 2018

Here's a nice diff that solves the mentioned problems:

diff --git a/PhysicsTools/NanoAOD/plugins/NPUTablesProducer.cc b/PhysicsTools/NanoAOD/plugins/NPUTablesProducer.cc
index 71cf2815a38..d1b821a17d3 100644
--- a/PhysicsTools/NanoAOD/plugins/NPUTablesProducer.cc
+++ b/PhysicsTools/NanoAOD/plugins/NPUTablesProducer.cc
@@ -4,9 +4,10 @@
 #include "FWCore/ParameterSet/interface/ConfigurationDescriptions.h"
 #include "FWCore/ParameterSet/interface/ParameterSetDescription.h"
 #include "DataFormats/NanoAOD/interface/FlatTable.h"
-#include "DataFormats/PatCandidates/interface/PackedGenParticle.h"
 #include "SimDataFormats/GeneratorProducts/interface/LHEEventProduct.h"
 #include "SimDataFormats/PileupSummaryInfo/interface/PileupSummaryInfo.h"
+#include "DataFormats/VertexReco/interface/Vertex.h"
+
 
 #include <vector>
 #include <iostream>
@@ -16,7 +17,7 @@ class NPUTablesProducer : public edm::global::EDProducer<> {
     public:
         NPUTablesProducer( edm::ParameterSet const & params ) :
 	   npuTag_(consumes<std::vector<PileupSummaryInfo>>(params.getParameter<edm::InputTag>("src"))),
-	   genTag_(consumes<edm::View<pat::PackedGenParticle>>(params.getParameter<edm::InputTag>("gensrc"))),
+	   pvTag_(consumes<std::vector<reco::Vertex>>(params.getParameter<edm::InputTag>("pvsrc"))),
 	   vz_(params.getParameter<std::vector<double>>("zbins"))
         {
             produces<nanoaod::FlatTable>();
@@ -27,18 +28,10 @@ class NPUTablesProducer : public edm::global::EDProducer<> {
         void produce(edm::StreamID id, edm::Event& iEvent, const edm::EventSetup& iSetup) const override {
             auto npuTab  = std::make_unique<nanoaod::FlatTable>(1, "Pileup", true);
 
-            
-	    edm::Handle<edm::View<pat::PackedGenParticle> >        genParticles;
-	    double refpvz = -1000.0;
-	    if ( iEvent.getByToken(genTag_, genParticles) ) {
-	      for ( auto genIt = genParticles->begin(); genIt != genParticles->end(); ++genIt ) {
-		if ( genIt->statusFlags().isHardProcess() ) {
-		  refpvz = genIt->vz();
-		  break;
-		}
-	      }
-	    }
-	    
+            edm::Handle<std::vector<reco::Vertex>> pvsIn;
+            iEvent.getByToken(pvTag_, pvsIn);
+            const double refpvz = (*pvsIn)[0].position().z();
+
 	    edm::Handle<std::vector<PileupSummaryInfo> > npuInfo;
             if (iEvent.getByToken(npuTag_, npuInfo)) {
 	      fillNPUObjectTable(*npuInfo, *npuTab, refpvz);
@@ -71,7 +64,7 @@ class NPUTablesProducer : public edm::global::EDProducer<> {
 		auto bin = std::lower_bound( vz_.begin(), vz_.end()-1, std::abs(zpositions.back()) );
 		if ( bin != vz_.end() && bin==zbin) gpudensity++;
 	      }
-	      gpudensity/=(20.0*( *(zbin+1) - *(zbin) ) ) ;
+	      gpudensity/=(20.0*( *(zbin) - *(zbin - 1) )) ;
 	    }
 	  }
 	  unsigned int eoot = 0;
@@ -93,14 +86,14 @@ class NPUTablesProducer : public edm::global::EDProducer<> {
         static void fillDescriptions(edm::ConfigurationDescriptions & descriptions) {
             edm::ParameterSetDescription desc;
             desc.add<edm::InputTag>("src", edm::InputTag("slimmedAddPileupInfo"))->setComment("tag for the PU information (vector<PileupSummaryInfo>)");
-            desc.add<edm::InputTag>("gensrc", edm::InputTag("packedGenParticles"))->setComment("tag for the packed GenParticles");
+            desc.add<edm::InputTag>("pvsrc", edm::InputTag("offlineSlimmedPrimaryVertices"))->setComment("tag for the PVs");
 	    desc.add<std::vector<double>> ("zbins", {})->setComment("Z bins to compute the generator-level number of PU vertices per mm");
             descriptions.add("puTable", desc);
         }
 
     protected:
        const edm::EDGetTokenT<std::vector<PileupSummaryInfo>> npuTag_;
-       const edm::EDGetTokenT<edm::View<pat::PackedGenParticle>> genTag_;
+       const edm::EDGetTokenT<std::vector<reco::Vertex>> pvTag_;
 
        const std::vector<double> vz_;
 };
diff --git a/PhysicsTools/NanoAOD/python/globals_cff.py b/PhysicsTools/NanoAOD/python/globals_cff.py
index 80fd1be339a..bfa17e72163 100644
--- a/PhysicsTools/NanoAOD/python/globals_cff.py
+++ b/PhysicsTools/NanoAOD/python/globals_cff.py
@@ -13,7 +13,7 @@ rhoTable = cms.EDProducer("GlobalVariablesTableProducer",
 
 puTable = cms.EDProducer("NPUTablesProducer",
         src = cms.InputTag("slimmedAddPileupInfo"),
-        gensrc = cms.InputTag("packedGenParticles"),
+        pvsrc = cms.InputTag("offlineSlimmedPrimaryVertices"),
         zbins = cms.vdouble( [0.0,1.7,2.6,3.0,3.5,4.2,5.2,6.0,7.5,9.0,12.0] )
 )

@rappoccio
Copy link
Author

Thanks a lot, although I'm getting that these are "corrupt patches" for some reason. Can you upload the file directly?

@rappoccio
Copy link
Author

Never mind, got it. Thanks a lot!

@ktht
Copy link

ktht commented Feb 27, 2019

You also need to add line

process.jercVars.srcJet="selectedUpdatedPatJetsWithDeepInfo"

after this line:

process.bJetVars.src="selectedUpdatedPatJetsWithDeepInfo"

Otherwise, BetaStarPackedCandidateVarProducer runs on slimmedJets and you'd get a runtime error.

@peruzzim
Copy link

peruzzim commented May 7, 2019

upstream features are available, this now needs rebasing on top of master-102X and adaptation to the changed order of jet analyzer modules there

@clelange
Copy link

clelange commented May 7, 2019

@rappoccio Could you please take care of this?
@ktht Then we should get the customiser on top of this into CMSSW as soon as possible so that we can get in touch with PPD regarding our custom NanoAOD production.

@peruzzim
Copy link

@rappoccio any news? thanks

@rappoccio
Copy link
Author

Sorry, I missed this. I will do it now.

@peruzzim
Copy link

sorry, could you please rebase on top of master-102X instead of merging? otherwise it becomes complicated to merge to upstream, the base is quite old

@rappoccio
Copy link
Author

Yes, trying to fix, sorry, I must have made a mistake in the rebase.

@rappoccio
Copy link
Author

(grr, sorry, still wrong)

@rappoccio
Copy link
Author

Grr. I keep getting these few commits in addition to mine. I think they are in CMSSW and not merged here?

@rappoccio
Copy link
Author

Dammit, still a conflict. Hang on.

@gpetruc-bot
Copy link

@gpetruc-bot
Copy link

Please update PhysicsTools/NanoAOD/python/nanoDQM_cfi.py: take this patch or run prepareDQM.py -d -u nano_file_mc.root, and then if needed adjust the plot range using some human common sense.

Copy link

@gpetruc-bot gpetruc-bot left a comment

Choose a reason for hiding this comment

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

Automatic test report for 860379

Code integration

Code checks passed for this PR

Please update PhysicsTools/NanoAOD/python/nanoDQM_cfi.py: take this patch or run prepareDQM.py -d -u nano_file_mc.root, and then if needed adjust the plot range using some human common sense.

Tests

  • Long test data102X (10000 events): passed, with differences; dqm plots: all, diff
  • Long test data80X (10000 events): passed, with differences; dqm plots: all, diff
  • Long test data80Xhip (3000 events): passed, with differences; dqm plots: all, diff
  • Long test data94X (10000 events): passed, with differences; dqm plots: all, diff
  • Long test data94X2016 (10000 events): passed, with differences; dqm plots: all, diff
  • Long test data94Xv2 (10000 events): passed, with differences; dqm plots: all, diff
  • Long test mc102X (9000 events): passed, with differences; dqm plots: all, diff
  • Long test mc80X (10000 events): passed, with differences; dqm plots: all, diff
  • Long test mc94X (10000 events): passed, with differences; dqm plots: all, diff
  • Long test mc94X2016 (9000 events): passed, with differences; dqm plots: all, diff
  • Long test mc94Xv2 (9000 events): passed, with differences; dqm plots: all, diff
  • Test mc_94Xv2: passed
  • Test mc_102X: passed
  • Test data_94X: passed
  • Test data_102X: passed

Disk size report

Sample kb/event ref kb/event diff
TTbar MC 102X 1.799 1.705 0.094 ( +5.5% )
TTbar MC 94Xv1 1.906 1.792 0.114 ( +6.3% )
TTbar MC 94Xv2 1.933 1.787 0.146 ( +8.2% )
TTbar MC 94X2016 1.724 1.653 0.071 ( +4.3% )
TTbar MC 80X 1.882 1.804 0.078 ( +4.3% )
Data 102X 0.945 0.893 0.052 ( +5.9% )
Data 94Xv1 0.900 0.832 0.068 ( +8.2% )
Data 80X 0.781 0.725 0.055 ( +7.6% )
Data 80X, Mu Run2016E 0.764 0.688 0.076 ( +11.0% )

@gpetruc-bot
Copy link

Copy link

@gpetruc-bot gpetruc-bot left a comment

Choose a reason for hiding this comment

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

Automatic test report for 861695

Code integration

Code checks passed for this PR

Tests

  • Long test data102X (10000 events): passed, no significant changes; dqm plots: all, diff
  • Long test data80X (10000 events): passed, no significant changes; dqm plots: all, diff
  • Long test data80Xhip (3000 events): passed, no significant changes; dqm plots: all, diff
  • Long test data94X (10000 events): passed, no significant changes; dqm plots: all, diff
  • Long test data94X2016 (10000 events): passed, no significant changes; dqm plots: all, diff
  • Long test data94Xv2 (10000 events): passed, no significant changes; dqm plots: all, diff
  • Long test mc102X (9000 events): passed, no significant changes; dqm plots: all, diff
  • Long test mc80X (10000 events): passed, no significant changes; dqm plots: all, diff
  • Long test mc94X (10000 events): passed, no significant changes; dqm plots: all, diff
  • Long test mc94X2016 (9000 events): passed, no significant changes; dqm plots: all, diff
  • Long test mc94Xv2 (9000 events): passed, no significant changes; dqm plots: all, diff
  • Test mc_94Xv2: passed
  • Test mc_102X: passed
  • Test data_94X: passed
  • Test data_102X: passed

Disk size report

Sample kb/event ref kb/event diff
TTbar MC 102X 1.811 1.780 0.031 ( +1.7% )
TTbar MC 94Xv1 1.915 1.885 0.029 ( +1.6% )
TTbar MC 94Xv2 1.947 1.913 0.034 ( +1.8% )
TTbar MC 94X2016 1.736 1.707 0.029 ( +1.7% )
TTbar MC 80X 1.892 1.865 0.027 ( +1.5% )
Data 102X 0.960 0.938 0.021 ( +2.2% )
Data 94Xv1 0.912 0.889 0.023 ( +2.6% )
Data 80X 0.791 0.771 0.020 ( +2.6% )
Data 80X, Mu Run2016E 0.772 0.754 0.018 ( +2.4% )

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

Successfully merging this pull request may close these issues.

None yet

5 participants