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

Add isolated charged hadron flag to RECO+AOD #18324

Merged
merged 3 commits into from Apr 20, 2017

Conversation

ahinzmann
Copy link
Contributor

@ahinzmann ahinzmann commented Apr 12, 2017

Followup of #18308

This PR adds a valueMap "chargedHadronPFTrackIsolation" for the PFCandisdates to RECO+AOD.
Charged hadrons with track pT > 1 GeV and rawCaloEnergy > 0.5 GeV are tagged on RECO.
With isolated charged hadrons, calorimeter response can be measured in data and simulation with fine granularity, enabling PF hadron calibration at AOD level.

runTheMatrix -l limited works.

It was checked that the variables in RECO+AOD are filled correctly with reasonable values.

Size with/without this change based on workflow 10024.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017 is summarized below:

New collection in RECO+AOD:
booledmValueMap_chargedHadronPFTrackIsolation__RECO. 677.8 278.7

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

RecoParticleFlow/Configuration
RecoParticleFlow/PFProducer

@perrotta, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@mmarionncern, @lgray, @rafaellopesdesa, @bachtis, @cbernet this is something you requested to watch as well.
@Muzaffar, @davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

@perrotta
Copy link
Contributor

@ahinzmann : is this supposed to replace #18308 ?
(Please clarify, and close any of them which is not needed any more)

@ahinzmann
Copy link
Contributor Author

#18308 is factorized in two PR. This is supposed to replace the AOD+RECO part of #18308. MiniAOD will follow after discussion at xpog

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 12, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19130/console Started: 2017/04/12 11:47

@slava77
Copy link
Contributor

slava77 commented Apr 12, 2017 via email

@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-18324/19130/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 21 differences found in the comparisons
  • DQMHistoTests: Total files compared: 23
  • DQMHistoTests: Total histograms compared: 1921525
  • DQMHistoTests: Total failures: 14573
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 1906778
  • DQMHistoTests: Total skipped: 173
  • DQMHistoTests: Total Missing objects: 0
  • Checked 94 log files, 14 edm output root files, 23 DQM output files

@cmsbuild
Copy link
Contributor

Pull request #18324 was updated. @perrotta, @cmsbuild, @slava77, @davidlange6 can you please check and sign again.

@ahinzmann
Copy link
Contributor Author

Renamed according to comment by @slava77 in #18308

@slava77
Copy link
Contributor

slava77 commented Apr 12, 2017

@cmsbuild please test

// get a view of our Candidates via the base candidates
typedef edm::View<reco::PFCandidate> PFCandidateView;
edm::Handle<PFCandidateView> Candidates;
evt.getByToken(Candidates_token, Candidates);
Copy link
Contributor

Choose a reason for hiding this comment

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

start local variables and data members with lower case letter. Capitalization is reserved for class names.


std::vector<bool> values;

for( PFCandidateView::const_iterator c = Candidates->begin(); c!=Candidates->end(); ++c) {
Copy link
Contributor

Choose a reason for hiding this comment

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

for range loop seems more appropriate for (auto cons& c : *candidates) {

if( theElements.empty() ) continue;
const reco::PFBlockRef blockRef = theElements[0].first;
const edm::OwnVector<reco::PFBlockElement>& elements = blockRef->elements();
for(unsigned int iEle=0; iEle<elements.size(); iEle++) { // Find the tracks in the block
Copy link
Contributor

Choose a reason for hiding this comment

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

similarly

 for (auto const& ele : elements){
...

@cmsbuild
Copy link
Contributor

Pull request #18324 was updated. @perrotta, @cmsbuild, @slava77, @davidlange6 can you please check and sign again.

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 19, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19234/console Started: 2017/04/19 10:41

@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-18324/19234/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 1642 differences found in the comparisons
  • DQMHistoTests: Total files compared: 23
  • DQMHistoTests: Total histograms compared: 1826307
  • DQMHistoTests: Total failures: 35953
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 1790180
  • DQMHistoTests: Total skipped: 173
  • DQMHistoTests: Total Missing objects: 0
  • Checked 94 log files, 14 edm output root files, 23 DQM output files

@slava77
Copy link
Contributor

slava77 commented Apr 19, 2017

+1

for #18324 62fd67c

  • new producer to make booledmValueMap_chargedHadronPFTrackIsolation is introduced as described; the branch appears up to AOD.
  • jenkins tests pass and AOD ouput in 1000.0 shows the new branch is in place
  • local test with 136.76 shows that the module is running at somewhat negligible CPU cost (0.5 ms/evt) and creates an output of about 100 bytes/event in size (one bool per about 2K PF candidates in this sample with about 50:1 false:true ratio).

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

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit d411fdf into cms-sw:master Apr 20, 2017
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

5 participants