NanoAOD prototype [RFC] #20626

gpetruc · 2017-09-22T11:53:03Z

Rebased #20563, removed data files, relocated dataformats, added first cmsDriver integration (but not runTheMatrix nor DQM yet).
Github doesn't allow me to re-open that PR because the branch was force-pushed, so I had to make a new one.

Some files are still to be relocated from PhysicsTools/NanoAOD to PhysicsTools/PatAlgos (or PatUtils)
Comment #20563 (comment) from @lgray also not implemented yet, it will probably come later in the follow-up of other multithreaded performance issues in https://hypernews.cern.ch/HyperNews/CMS/get/edmFramework/3791.html

@arizzi @peruzzim @emanueledimarco

…tag v1.0 except data files

* new step -s NANO * new event contents NANOAOD, NANOAODSIM * use NanoAODOutputModule if datatier == "NANO", otherwise use PoolOutputModule

cmsbuild · 2017-09-22T11:53:26Z

The code-checks are being triggered in jenkins.

cmsbuild · 2017-09-22T11:57:12Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20626/907

cmsbuild · 2017-09-22T11:57:31Z

A new Pull Request was created by @gpetruc (Giovanni Petrucciani) for master.

It involves the following packages:

Configuration/Applications
DataFormats/NanoAOD
DataFormats/PatCandidates
PhysicsTools/NanoAOD
RecoEgamma/PhotonIdentification

The following packages do not have a category, yet:

DataFormats/NanoAOD
PhysicsTools/NanoAOD
Please create a PR for https://github.com/cms-sw/cms-bot/blob/master/categories.py to assign category

@perrotta, @monttj, @cmsbuild, @franzoni, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @Sam-Harper, @jainshilpi, @rovere, @lgray, @Martin-Grunewald, @gouskos, @cbernet this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

slava77 · 2017-09-22T12:31:34Z

On 9/22/17 4:53 AM, Giovanni Petrucciani wrote: Github doesn't allow me to re-open that PR because the branch was force-pushed, so I had to make a new one.

this is the price you pay for closing the PR in attempt to make less noise An open PR can still have its' topic branch force-pushed.

cmsbuild · 2017-10-06T09:18:46Z

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

cmsbuild · 2017-10-06T12:10:09Z

+1
Tested at: 144f8b9
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20626/23490/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
4390117
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20626/23490/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20626/23490/git-merge-result

cmsbuild · 2017-10-06T12:10:11Z

Comparison job queued.

cmsbuild · 2017-10-06T13:42:57Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 26
DQMHistoTests: Total histograms compared: 2761427
DQMHistoTests: Total failures: 246
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2760994
DQMHistoTests: Total skipped: 187
DQMHistoTests: Total Missing objects: 0
Checked 107 log files, 10 edm output root files, 26 DQM output files

slava77 · 2017-10-09T16:28:26Z

DataFormats/PatCandidates/src/PackedCandidate.cc

@@ -150,7 +150,8 @@ float pat::PackedCandidate::dxy(const Point &p) const {
 float pat::PackedCandidate::dz(const Point &p) const {
    maybeUnpackBoth();
    const float phi = float(p4_.load()->Phi())+dphi_;
-    return (vertex_.load()->Z()-p.Z())  - ((vertex_.load()->X()-p.X()) * std::cos(phi) + (vertex_.load()->Y()-p.Y()) * std::sin(phi)) * p4_.load()->Pz()/p4_.load()->Pt();
+    const float pzpt = deta_ ? std::sinh(etaAtVtx()) : p4_.load()->Pz()/p4_.load()->Pt();
+    return (vertex_.load()->Z()-p.Z())  - ((vertex_.load()->X()-p.X()) * std::cos(phi) + (vertex_.load()->Y()-p.Y()) * std::sin(phi)) * pzpt;


changes in this file are merged already as a part of #20491
it looks like in this PR 60f6f31 comes before 852157f while in #20491 the order is reversed and there is also 5db31d6 in between, which is not also in this PR.
Some cleanup of the history would be nice.

slava77 · 2017-10-09T16:35:01Z

RecoEgamma/PhotonIdentification/plugins/PhotonIDValueMapProducer.cc

@@ -42,13 +42,13 @@ class PhotonIDValueMapProducer : public edm::stream::EDProducer<> {
  public:

  explicit PhotonIDValueMapProducer(const edm::ParameterSet&);
-  ~PhotonIDValueMapProducer();


changes in this file are in the release already (the same commit IDs and in the same order).

gpetruc · 2017-10-09T16:51:29Z

Hi Slava,

#20626 (and #20563 before it) was done on top of #20491 so it's no surprise that you find the same commits in the two; if you see them in the same order or you see other commits in between it's just the github display that doesn't do the sorting properly when mapping a graph into a 1d timeline (since it's the same commits and not copies, by definition they are in the same order in the history graph, as it's the same graph)

New developments have been already done by many people on top of this PR during, so I'll not rebase it as that would mess up the history of all the rest remaining developments.

slava77 · 2017-10-09T17:37:23Z

On 10/9/17 9:51 AM, Giovanni Petrucciani wrote: Hi Slava, #20626 <#20626> (and #20563 <#20563> before it) was done on top of #20491 <#20491> so it's no surprise that you find the same commits in the two; if you see them in the same order or you see other commits in between it's just the github display that doesn't do the sorting properly when mapping a graph into a 1d timeline (since it's the same commits and not copies, by definition they are in the same order in the history graph, as it's the same graph)

you are right, if commit IDs are the same there can be only one actual order. Consider the comments as FYI for myself and probably others who may notice this.

…

New developments have been already done by many people on top of this PR during, so I'll not rebase it as that would mess up the history of all the rest remaining developments. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20626 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEdcbtC8kWcxpnFhO9S-XzxJMO2gUk03ks5sqk8TgaJpZM4Pgmh->.

arizzi · 2017-10-09T18:00:28Z

dear all,
given that

this is PR is large and it may take a lot of time to fully review the code
it doesn't by design (and by tests results) affect any existing workflow
all technical tests are ok
the code is far from final, being a prototype, but is nevertheless urgently needed in a release
we have a growing pipeline of developments (the development is very active being a prototype)

I propose that we proceed as follow:

merge now
open github issues on anything that you think should be improved with asignment to me&giovanni and we guarantee that we will address them all in the very next PRs

slava77 · 2017-10-09T21:35:41Z

PhysicsTools/NanoAOD/plugins/VIDNestedWPBitmapProducer.cc

+    isInit_(false)
+  {
+    auto vwp = iConfig.getParameter<std::vector<std::string>>("WorkingPoints");
+    for (auto wp : vwp) {


auto const &

slava77 · 2017-10-09T21:39:27Z

PhysicsTools/NanoAOD/plugins/LHETablesProducer.cc

+                lheVpt = std::hypot( pup[v.first][0] + pup[v.second][0], pup[v.first][1] + pup[v.second][1] ); 
+            }
+
+            out.addColumnValue<uint8_t>("Njets", lheNj, "Number of jets (partons) at LHE step", nanoaod::FlatTable::UInt8Column);


it doesn't look normal that every event we have to specify types and documentation description string.

it's an overhead but it gives us the flexibility we need (eg allowing the schema to depend on the configuration) while keeping the output module generic and the code simple (avoid percolating stuff through provenance or run/lumi branches).
we may re-engineer things in the future if the overhead becomes large (now i/o and other physics object code dominate the costs)

It's not just the overhead: this implementation apparently allows changes in format event by event, which sounds dangerous.

the nanoaod format is by design less structured.
We made some design choice balancing risks and benefits. We would be happy to discuss this design in any of the XPOG meeting where the NanoAOD format is discussed.

I'm not sure what this response means.
Does the Nano format support event-by-event changes in branch/table content?
Please clarify.

in some cases (not in the one above) it does. E.g. HLT bits are allowed to change from one event to another and proper (back)filling of the branches is implemented.
As for the rest, while it is not meant to change event by event, there is no "configuration" of the event content specified explicitelly anywhere (not at compile time in the DataFormats, not explicitely at config time).
The configuration of the branches to write is configured basically at first event.

I do not think this discussion belong to a github PR. If you want to understand why we designed it so, we can have a chat on this topic.

slava77 · 2017-10-09T21:53:28Z

PhysicsTools/PatAlgos/plugins/BaseMVAValueMapProducer.h

+	reader_.AddVariable(v,(&values_.front())+i);
+	i++;
+      }
+//      reader_.BookMVA(name_,iConfig.getParameter<edm::FileInPath>("weightFile").fullPath() );


is this commented out code needed?
Please remove or add comments inline in the code why the commented out block is relevant

slava77 · 2017-10-09T22:00:21Z

PhysicsTools/PatAlgos/plugins/PATObjectCrossLinker.cc

@@ -0,0 +1,229 @@
+// -*- C++ -*-
+//
+// Package:    PhysicsTools/NanoAOD


this doesn't match the file location.
full path given in the line with \class doesn't match either.

slava77 · 2017-10-09T22:19:07Z

+1

for #20626 144f8b9

mostly nano-AOD package/algorithm developments; several helper tools relevant for PAT object manipulation were added to PhysicsTools/PatAlgos
jenkins tests pass (the unit test with a nanoAOD workflow is running OK)
local test on 3K events with ttbar PU shows somewhat reasonable technical performance
- output size is about 3.6 kiB, roughly as expected
- memory peak with one thread is about 1.1 GiB and about 1.3 GiB with 8 threads
- CPU efficiency with 8 threads is ~60%
- CPU use per event is ~0.1 s/event (compared to about 1.6 s/event on the same machine to make miniAOD or about 17 s/event to make RECO/AOD)
  - about 50% of the CPU is taken by photonIDValueMapProducer. Some optimization here could help to speedup the full workflow.

commented out code noted in PhysicsTools/PatAlgos can be cleaned up in a follow up PR

arizzi · 2017-10-11T17:51:30Z

@davidlange6 any chance merging this before the next IB ?

davidlange6 · 2017-10-11T19:37:02Z

merge

gpetruc and others added 14 commits September 18, 2017 10:18

Merged refs/pull/20491/head from repository cms-sw with cms-merge-topic

1d61603

Import NanoAOD prototype code @ https://github.com/gpetruc/NanoAOD.git …

edae6c2

…tag v1.0 except data files

scram code-checks of PhysicsTools/NanoAOD

06da61b

add support for gzipped xml

29879a4

aply JECs before flight

010e333

add L2L3 and sort the mess

372bb17

add some basic in-time pileup info

1f8f91d

reduce the number of genparticles

f27f976

include suggestion from @bendavid

89d6897

Set the default NanoAOD outputCommands to be the right ones

c09afb6

Minimalistic cmsDriver integration:

d7dc162

* new step -s NANO * new event contents NANOAOD, NANOAODSIM * use NanoAODOutputModule if datatier == "NANO", otherwise use PoolOutputModule

add tooltip to treesize.py and option to print doc stuff

22ee0e2

better treesize.py doc

19d62c4

Relocate dataformats in DataFormats/NanoAOD

6faa025

cmsbuild added this to the CMSSW_9_4_X milestone Sep 22, 2017

cmsbuild added analysis-pending code-checks-pending comparison-pending new-package-pending operations-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Sep 22, 2017

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 22, 2017

cmsbuild added tests-started and removed tests-pending labels Oct 6, 2017

cmsbuild added tests-approved and removed tests-started labels Oct 6, 2017

cmsbuild added comparison-available and removed comparison-pending labels Oct 6, 2017

slava77 reviewed Oct 9, 2017

View reviewed changes

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Oct 9, 2017

cmsbuild added orp-approved and removed orp-pending labels Oct 11, 2017

cmsbuild merged commit 70922db into cms-sw:master Oct 11, 2017

arizzi mentioned this pull request Oct 12, 2017

NanoAOD: runTheMatrix and cmsDriver support #20905

Merged

arizzi mentioned this pull request Jun 12, 2018

NanoAOD update 10XY version (master version of #23445) #23446

Merged

slava77 mentioned this pull request Jul 25, 2020

Illegal include of PhysicsTools/PatAlgos/plugins in PhysicsTools/NanoAOD #30910

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NanoAOD prototype [RFC] #20626

NanoAOD prototype [RFC] #20626

gpetruc commented Sep 22, 2017

cmsbuild commented Sep 22, 2017

cmsbuild commented Sep 22, 2017

cmsbuild commented Sep 22, 2017

slava77 commented Sep 22, 2017 via email

cmsbuild commented Oct 6, 2017 •

edited

cmsbuild commented Oct 6, 2017

cmsbuild commented Oct 6, 2017

cmsbuild commented Oct 6, 2017

slava77 Oct 9, 2017

slava77 Oct 9, 2017

arizzi Oct 9, 2017

gpetruc commented Oct 9, 2017

slava77 commented Oct 9, 2017 via email

arizzi commented Oct 9, 2017

slava77 Oct 9, 2017

slava77 Oct 9, 2017

gpetruc Oct 10, 2017

slava77 Oct 10, 2017

arizzi Oct 10, 2017

slava77 Oct 10, 2017

arizzi Oct 10, 2017

slava77 Oct 9, 2017

slava77 Oct 9, 2017

slava77 commented Oct 9, 2017

arizzi commented Oct 11, 2017

davidlange6 commented Oct 11, 2017

NanoAOD prototype [RFC] #20626

NanoAOD prototype [RFC] #20626

Conversation

gpetruc commented Sep 22, 2017

cmsbuild commented Sep 22, 2017

cmsbuild commented Sep 22, 2017

cmsbuild commented Sep 22, 2017

slava77 commented Sep 22, 2017 via email

cmsbuild commented Oct 6, 2017 • edited

cmsbuild commented Oct 6, 2017

cmsbuild commented Oct 6, 2017

cmsbuild commented Oct 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpetruc commented Oct 9, 2017

slava77 commented Oct 9, 2017 via email

arizzi commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slava77 commented Oct 9, 2017

arizzi commented Oct 11, 2017

davidlange6 commented Oct 11, 2017

cmsbuild commented Oct 6, 2017 •

edited