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

GenJetFlavourInfos in MiniAOD and Change in JetFlavour of LightJets to physicsPartons in CMSSW 92X #19048

Merged
merged 22 commits into from Aug 25, 2017

Conversation

Andrej-CMS
Copy link
Contributor

Dear all, @arizzi

This PR introduces two changes to the MiniAODSim format.

  1. The GenJetFlavourInfoPreserver tool passes the GenJetFlavour information of the original AK4GenJets in the AODSim to the slimmedGenJets in the MiniAODSim. The new GenJetFlavour information is called "slimmedGenJetsFlavourInfos" and is stored in the MiniAOD. This collection is of the class reco::JetFlavourInfoMatchingCollection and the object consists out of a pair of refvectors.

  2. For the case where no heavy flavour hadron is clustered into the GenJet, a definition based on the "physicsPartons" is chosen. physicsPartons now contain the full shower. Changes were proposed by the QGL group.

Kind regards,

Andrej

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Andrej-CMS for master.

It involves the following packages:

PhysicsTools/JetMCAlgos
PhysicsTools/PatAlgos

@perrotta, @cmsbuild, @slava77, @monttj, @davidlange6 can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @imarches, @ahinzmann, @acaudron, @mmarionncern, @rappoccio, @jdolen, @nhanvtran, @gpetruc, @gkasieczka, @schoef, @ferencek, @mverzett, @mariadalfonso, @pvmulder, @seemasharmafnal, @JyothsnaKomaragiri this is something you requested to watch as well.
@davidlange6 you are the release manager for this.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented May 31, 2017

@cmsbuild 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/20266/console Started: 2017/05/31 21:36

@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-19048/20266/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 3240 differences found in the comparisons
  • DQMHistoTests: Total files compared: 23
  • DQMHistoTests: Total histograms compared: 1786072
  • DQMHistoTests: Total failures: 48698
  • DQMHistoTests: Total nulls: 16
  • DQMHistoTests: Total successes: 1737185
  • DQMHistoTests: Total skipped: 173
  • DQMHistoTests: Total Missing objects: 0
  • Checked 94 log files, 14 edm output root files, 23 DQM output files

@ahinzmann
Copy link
Contributor

@Andrej-CMS Do you have some plots or numbers that show how this changes the flavor composition of the leading jet on a RelVal sample (e.g. z+jets should have ~60% quarks, qcd should have 50-70% gluons)?

@@ -254,7 +254,7 @@ HadronAndPartonSelector::produce(edm::Event& iEvent, const edm::EventSetup& iSet
partonSelector_->run(particles,partons);
for(reco::GenParticleCollection::const_iterator it = particles->begin(); it != particles->end(); ++it)
{
if( !(it->status()==3 || (( partonMode_=="Pythia8" ) && (it->status()==23)))) continue;
// if( !(it->status()==3 || (( partonMode_=="Pythia8" ) && (it->status()==23)))) continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Dear @Andrej-CMS,

I am wondering if commenting out this has any consequences not only in terms of file size but also in other places where HadronAndSelector is being used.
It is because if I understand correctly, without this condition, this will contain all partons including status 2 and also stable final particles. That will be a lot of particles. This will also introduce ambiguity issue.

Best Regards,
Taejeong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @monttj ,
These changes were requested by the QGL Group, i.e. Paolo Azzuri. Unfortunately, I cannot find him on GitHub to comment on this.

You are right, the physicspartons will contain the whole chain. I will provide RelVal comparisons as soon as possible.

Kind regards,
Andrej

Copy link
Contributor

Choose a reason for hiding this comment

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

can you make that configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I will do that.

Kind regards,
Andrej

@slava77
Copy link
Contributor

slava77 commented Jun 5, 2017

Please note: this PR will not be merged until some validation plots/slides are provided.

@@ -254,7 +254,7 @@ HadronAndPartonSelector::produce(edm::Event& iEvent, const edm::EventSetup& iSet
partonSelector_->run(particles,partons);
for(reco::GenParticleCollection::const_iterator it = particles->begin(); it != particles->end(); ++it)
{
if( !(it->status()==3 || (( partonMode_=="Pythia8" ) && (it->status()==23)))) continue;
// if( !(it->status()==3 || (( partonMode_=="Pythia8" ) && (it->status()==23)))) continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like this part will be modified to become configurable.
If it would not, the following applies:
is this commented out code needed?
Please remove or add comments inline in the code why the commented out block is relevant

@@ -4,7 +4,7 @@
jets = cms.InputTag("ak4PFJets"),
bHadrons = cms.InputTag("selectedHadronsAndPartons","bHadrons"),
cHadrons = cms.InputTag("selectedHadronsAndPartons","cHadrons"),
partons = cms.InputTag("selectedHadronsAndPartons","algorithmicPartons"),
partons = cms.InputTag("selectedHadronsAndPartons","physicsPartons"),
Copy link
Contributor

Choose a reason for hiding this comment

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

should the same be applied to other available instances of JetFlavourClustering in this package?
There is one in AK5

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2713569
  • DQMHistoTests: Total failures: 276
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2713104
  • DQMHistoTests: Total skipped: 189
  • DQMHistoTests: Total Missing objects: 0
  • Checked 107 log files, 14 edm output root files, 26 DQM output files

@slava77
Copy link
Contributor

slava77 commented Aug 13, 2017

I'm looking at the changes:
(this it a ttbar sample no PU for 2017 setup)
all_mini_origvssign922_ttbar13tev2017wf10024p0c_recojetflavourinfomatchingcollection_slimmedgenjetsflavourinfos__reco_obj_data__m_hadronflavour
shouldn't there be something between 1 and 3 in this variable?

the parton flavor apparently covers the relevant range
all_mini_origvssign922_ttbar13tev2017wf10024p0c_recojetflavourinfomatchingcollection_slimmedgenjetsflavourinfos__reco_obj_data__m_partonflavour

@ahinzmann
Copy link
Contributor

@slava77 Hadron flavour only looks for B-hadrons, thus cannot detect light quarks.

@slava77
Copy link
Contributor

slava77 commented Aug 14, 2017

+1

for #19048 19314ca

  • changes are in line with the description
  • jenkins tests pass and comparisons with baseline show differences only in /JetValidation/slimmedJets PartonFlavor as expected
  • local tests to compare new variables show expected behavior

@slava77
Copy link
Contributor

slava77 commented Aug 15, 2017

@dmitrijus @monttj
dqm and analysis signatures are needed for this PR.
Please check and sign or suggest changes.

@slava77
Copy link
Contributor

slava77 commented Aug 16, 2017

@dmitrijus @monttj
dqm and analysis signatures are needed for this PR.
Please check and sign or suggest changes.
Thank you.

@slava77
Copy link
Contributor

slava77 commented Aug 17, 2017

@dmitrijus @monttj
dqm and analysis signatures are needed for this PR.
Please check and sign or suggest changes.
Thank you.

@davidlange6 please check.

@slava77
Copy link
Contributor

slava77 commented Aug 21, 2017

to continue my weekdaily ping

@dmitrijus @monttj
dqm and analysis signatures are needed for this PR.
Please check and sign or suggest changes.
Thank you.

@davidlange6 please check.

@slava77
Copy link
Contributor

slava77 commented Aug 22, 2017

code-checks

the last checks went outside of the scope of the PR

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-19048/246

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/PR-19048/246/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/PR-19048/246/git-diff.patch | patch -p1

In future, you can run scram build code-checks to apply code checks

@slava77
Copy link
Contributor

slava77 commented Aug 22, 2017

to continue my weekdaily ping on day 8 after no activity

@dmitrijus @monttj
dqm and analysis signatures are needed for this PR.

@davidlange6 please check and merge with missing signatures or suggest changes.
Thank you.

@dmitrijus
Copy link
Contributor

+1

@slava77
Copy link
Contributor

slava77 commented Aug 25, 2017

@davidlange6 please check and merge with missing AT signature or suggest changes.

Thank you.

@davidlange6
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit c61529f into cms-sw:master Aug 25, 2017
edm::Ref<std::vector<reco::GenJet> > slimmedGenJetRef;

int i = -1;
for (auto const& genJet : *genJets){
Copy link
Contributor

Choose a reason for hiding this comment

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

hi @Andrej-CMS @slava77 @perrotta - this brings a warning as genJet is not actually used. Please change to a loop over i if genJets->refAt(i) is not the same as genJet in the loop below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @davidlange6 ,

I just pushed the changes to the initial branch on my repository: https://github.com/Andrej-CMS/cmssw/tree/JetFlavourMiniAOD92X

Since the changes here are already merged, do I need to make a new PR?

Kind regards,
Andrej

@davidlange6
Copy link
Contributor

davidlange6 commented Aug 28, 2017 via email

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