Add energy regression and particle identification to RecoHGCal/TICL (reopened) #27917

riga · 2019-09-02T15:08:29Z

PR description

This PR adds energy regression and particle identification for trackster objects to the TrackstersProducer of the RecoHGCal/TICL package. Both the regressed energy and ID probabilities for different types of particles are the outputs of a single neural network, stored as a TensorFlow model. The first version of this model is proposed in

cms-data/RecoHGCal-TICL#1.

This PR is a clean version of #27634. The changes since the last review comments can be found here.

Performance

Tested with workflow 20634.0 (TTbar_14TeV_TuneCUETP8M1_2026D41PU with 200 PU).

The CPU runtime for inference scales linearly with the batch size, i.e., the number of tracksters, as expected for the no_threads TensorFlow session. The average runtime per trackster is ~0.6 ms (see slope below). The time includes trackster selection, preprocessing and filling of input data, the actual inference, and handling of results (== PatternRecognitionbyCA::energyRegressionAndID).

The memory consumption, obtained using the SimpleMemoryCheck with and without running the regression and ID, is 178 MB for a maximum batch size of ~750 tracksters.

To get an estimate of the memory used per event, I queried VmRSS of /proc/self/status (link) at the beginning and at the end of PatternRecognitionbyCA::energyRegressionAndID, and also before and after loading the graph and setting up the session.

Load graph: + 3.6 MB
Load session: +7.2 MB
energyRegressionAndID:

cmsbuild · 2019-09-02T15:08:59Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-09-02T15:15:04Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27917/11736

This PR adds an extra 24KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DataFormats/HGCalReco/interface/Trackster.h modified in PR(s): HGCAL: 1st version of hadronic reconstruction #27819
- File DataFormats/HGCalReco/src/classes_def.xml modified in PR(s): HGCAL: 1st version of hadronic reconstruction #27819
- File RecoHGCal/TICL/interface/PatternRecognitionAlgoBase.h modified in PR(s): HGCAL: 1st version of hadronic reconstruction #27819
- File RecoHGCal/TICL/plugins/BuildFile.xml modified in PR(s): HGCAL: 1st version of hadronic reconstruction #27819
- File RecoHGCal/TICL/plugins/PatternRecognitionbyCA.cc modified in PR(s): HGCAL: 1st version of hadronic reconstruction #27819
- File RecoHGCal/TICL/plugins/PatternRecognitionbyCA.h modified in PR(s): HGCAL: 1st version of hadronic reconstruction #27819
- File RecoHGCal/TICL/plugins/PatternRecognitionbyMultiClusters.h modified in PR(s): HGCAL: 1st version of hadronic reconstruction #27819
- File RecoHGCal/TICL/plugins/TrackstersProducer.cc modified in PR(s): HGCAL: 1st version of hadronic reconstruction #27819

cmsbuild · 2019-09-02T15:15:35Z

A new Pull Request was created by @riga (Marcel R.) for master.

It involves the following packages:

DataFormats/HGCalReco
RecoHGCal/TICL

@perrotta, @cmsbuild, @kpedro88, @slava77 can you please review it and eventually sign? Thanks.
@felicepantaleo, @apsallid, @rovere, @clelange, @hatakeyamak this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

perrotta · 2019-09-02T15:59:21Z

please test workflow 20493.52 with cms-sw/cmsdist#5120, cms-data/RecoHGCal-TICL#1

cmsbuild · 2019-09-02T15:59:49Z

The tests are being triggered in jenkins.
Tested with other pull request(s) cms-sw/cmsdist#5120,cms-data/RecoHGCal-TICL#1
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2316/console Started: 2019/09/02 18:01

VinInn · 2019-09-02T16:27:47Z

DataFormats/HGCalReco/interface/Trackster.h

+    std::array<float, 8> id_probabilities;
+
+    // convenience method to return the ID probability for a certain particle type
+    inline float id_probability(ParticleType type) {


add const

VinInn · 2019-09-02T16:33:15Z

RecoHGCal/TICL/plugins/TrackstersProducer.cc

+}
+
+void TrackstersProducer::globalEndJob(TrackstersCache* cache) {
+  if (cache->eidGraphDef != nullptr) {


no need to protect a delete

VinInn · 2019-09-02T16:34:53Z

RecoHGCal/TICL/plugins/PatternRecognitionbyCA.cc

+  const TrackstersCache *trackstersCache = dynamic_cast<const TrackstersCache *>(cache);
+  if (trackstersCache->eidGraphDef != nullptr) {
+    eidSession_ = tensorflow::createSession(trackstersCache->eidGraphDef);
+  }


When the eidGraphDef in the cache is not set (which happens when no graph path was set) the eidSession_ should not be set as well and the inference is not performed.

how does this distinguish a bug upstream from a desired silent do nothing behavior?
The code should not be allowed to silently skip when it was expected to do something.

VinInn · 2019-09-02T16:36:32Z

RecoHGCal/TICL/plugins/PatternRecognitionbyCA.cc

+
+  // mount the tensorflow graph onto the session when set
+  const TrackstersCache *trackstersCache = dynamic_cast<const TrackstersCache *>(cache);
+  if (trackstersCache->eidGraphDef != nullptr) {


actually one has to test if the dynamic_cast failed...

VinInn · 2019-09-02T16:41:26Z

RecoHGCal/TICL/plugins/PatternRecognitionbyCA.cc

  theGraph_ = std::make_unique<HGCGraph>();
  min_cos_theta_ = conf.getParameter<double>("min_cos_theta");
  min_cos_pointing_ = conf.getParameter<double>("min_cos_pointing");
  missing_layers_ = conf.getParameter<int>("missing_layers");
  min_clusters_per_ntuplet_ = conf.getParameter<int>("min_clusters_per_ntuplet");
  max_delta_time_ = conf.getParameter<double>("max_delta_time");
+  eidInputName_ = conf.getParameter<std::string>("eid_input_name");


all configurable data members should be declared const and initialized as
theGraph_(std::make_unique<HGCGraph>()), etc

Please make const all all configurable data members

VinInn · 2019-09-02T16:50:12Z

RecoHGCal/TICL/plugins/PatternRecognitionbyCA.cc

+      for (int k = seenClusters[j]; k < eidNClusters_; k++) {
+        float *features = &input.tensor<float, 4>()(i, j, k, 0);
+        for (int l = 0; l < eidNFeatures_; l++, features++) {
+          *features = 0.f;


why not
*(features++) as everywhere else?
just for consistency...

cmsbuild · 2019-09-02T17:53:30Z

+1
Tested at: 17525a0
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b9a975/2316/summary.html

cmsbuild · 2019-09-02T17:53:39Z

Comparison job queued.

cmsbuild · 2019-09-23T11:58:03Z

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

riga · 2019-09-23T12:03:29Z

@perrotta I made all members const. Also, I ran igprof and browsed through the results. This is the output for the added energyRegressionAndID method:

Fyi: Running igprof-navigator on an lxplus machine (in a CMSSW env) returned an empty page in my browser and the process complains about a "SQLite header and source version mismatch". In the end, I used

docker run --rm -ti -p 8080:8080 -v `pwd`/thefile.sql3:/thefile.sql3 igprof/igprof igprof-navigator /thefile.sql3

Maybe this can go on the Twiki page you pointed me to in case other people have the same issue.

perrotta · 2019-09-23T12:10:09Z

please test workflow 20493.52 with cms-sw/cmsdist#5120, cms-data/RecoHGCal-TICL#1

cmsbuild · 2019-09-23T12:10:36Z

The tests are being triggered in jenkins.
Tested with other pull request(s) cms-sw/cmsdist#5120,cms-data/RecoHGCal-TICL#1
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2621/console Started: 2019/09/23 14:12

cmsbuild · 2019-09-23T15:09:23Z

+1
Tested at: 7f663ca
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b9a975/2621/summary.html

cmsbuild · 2019-09-23T15:09:35Z

Comparison job queued.

cmsbuild · 2019-09-23T17:39:32Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2958092
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2957750
DQMHistoTests: Total skipped: 341
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 145 log files, 15 edm output root files, 34 DQM output files

perrotta · 2019-09-23T20:05:49Z

+1

Energy regression and particle id implemented for HGCal trackster objects
When enabled, cpu and memory consumption shown to be acceptable even at 200 PU
TrackstersProducer left configured to skip regression and id: therefore no change is expected in the default config: jenkins tests pass and show no differences

fabiocos · 2019-09-24T13:32:24Z

@kpedro88 could you please check this PR?

kpedro88 · 2019-09-24T15:54:20Z

+upgrade

cmsbuild · 2019-09-24T15:55:01Z

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 will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

fabiocos · 2019-09-25T20:43:33Z

+1

cmsbuild added this to the CMSSW_11_0_X milestone Sep 2, 2019

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending upgrade-pending labels Sep 2, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 2, 2019

This was referenced Sep 2, 2019

Initial reference for RecoHGCal-TICL data repo cms-sw/cmsdist#5120

Merged

Initial commit with CNN-based energy regression and ID tf model. cms-data/RecoHGCal-TICL#1

Merged

cmsbuild added requires-external tests-started and removed tests-pending labels Sep 2, 2019

VinInn reviewed Sep 2, 2019

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Sep 2, 2019

cmsbuild added requires-external tests-started and removed tests-pending labels Sep 23, 2019

cmsbuild added tests-approved and removed tests-started labels Sep 23, 2019

cmsbuild added comparison-available and removed comparison-pending labels Sep 23, 2019

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Sep 23, 2019

cmsbuild added fully-signed upgrade-approved and removed pending-signatures upgrade-pending labels Sep 24, 2019

cmsbuild added orp-approved and removed orp-pending labels Sep 25, 2019

cmsbuild merged commit 81891b0 into cms-sw:master Sep 25, 2019

slava77 mentioned this pull request Apr 2, 2020

[HGCAL] TICL in reconstruction #29081

Merged

fwyzard mentioned this pull request May 12, 2020

Fix features order #29799

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add energy regression and particle identification to RecoHGCal/TICL (reopened) #27917

Add energy regression and particle identification to RecoHGCal/TICL (reopened) #27917

riga commented Sep 2, 2019 •

edited

Loading

cmsbuild commented Sep 2, 2019

cmsbuild commented Sep 2, 2019

cmsbuild commented Sep 2, 2019

perrotta commented Sep 2, 2019

cmsbuild commented Sep 2, 2019 •

edited

Loading

VinInn Sep 2, 2019

riga Sep 3, 2019

VinInn Sep 2, 2019

riga Sep 3, 2019

VinInn Sep 2, 2019

riga Sep 3, 2019

slava77 Sep 5, 2019

VinInn Sep 2, 2019

riga Sep 3, 2019

VinInn Sep 2, 2019 •

edited

Loading

perrotta Sep 22, 2019

riga Sep 23, 2019

VinInn Sep 2, 2019 •

edited

Loading

riga Sep 3, 2019

cmsbuild commented Sep 2, 2019

cmsbuild commented Sep 2, 2019

cmsbuild commented Sep 23, 2019

riga commented Sep 23, 2019

perrotta commented Sep 23, 2019

cmsbuild commented Sep 23, 2019 •

edited

Loading

cmsbuild commented Sep 23, 2019

cmsbuild commented Sep 23, 2019

cmsbuild commented Sep 23, 2019

perrotta commented Sep 23, 2019

fabiocos commented Sep 24, 2019

kpedro88 commented Sep 24, 2019

cmsbuild commented Sep 24, 2019

fabiocos commented Sep 25, 2019

Add energy regression and particle identification to RecoHGCal/TICL (reopened) #27917

Add energy regression and particle identification to RecoHGCal/TICL (reopened) #27917

Conversation

riga commented Sep 2, 2019 • edited Loading

PR description

Performance

cmsbuild commented Sep 2, 2019

cmsbuild commented Sep 2, 2019

cmsbuild commented Sep 2, 2019

perrotta commented Sep 2, 2019

cmsbuild commented Sep 2, 2019 • edited Loading

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

VinInn Sep 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinInn Sep 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Sep 2, 2019

cmsbuild commented Sep 2, 2019

cmsbuild commented Sep 23, 2019

riga commented Sep 23, 2019

perrotta commented Sep 23, 2019

cmsbuild commented Sep 23, 2019 • edited Loading

cmsbuild commented Sep 23, 2019

cmsbuild commented Sep 23, 2019

cmsbuild commented Sep 23, 2019

perrotta commented Sep 23, 2019

fabiocos commented Sep 24, 2019

kpedro88 commented Sep 24, 2019

cmsbuild commented Sep 24, 2019

fabiocos commented Sep 25, 2019

riga commented Sep 2, 2019 •

edited

Loading

cmsbuild commented Sep 2, 2019 •

edited

Loading

VinInn Sep 2, 2019 •

edited

Loading

VinInn Sep 2, 2019 •

edited

Loading

cmsbuild commented Sep 23, 2019 •

edited

Loading