TICL: Create PF candidates as output of TICL #28116

steggema · 2019-10-04T16:00:40Z

PR description:

This PR prepares the integration of TICL into CMS Particle Flow by creating PFCandidates that can subsequently be integrated in the central CMS PF collection. It does so with the help of an intermediate data format, the TICLCandidate, by converting the TICL output (a list of tracksters) into TICLCandidates, and subsequently the TICLCandidates into regular PFCandidates. It also adds DQM plots for the such-produced PFCandidates.

Presentations were given in a RECO/AT meeting, https://indico.cern.ch/event/841640/contributions/3539177/attachments/1896768/3129567/2019_08_23_reco.pdf, and in an HGCAL DPG meeting, https://indico.cern.ch/event/849004/contributions/3567562/attachments/1910534/3156782/2019_09_18_hgcal_dpg.pdf

PR validation:

The workflow has been validated by enabling TICL as defined in RecoHGCal/TICL/python/ticl_iterations.py and checking the DQM output.

if this PR is a backport please specify the original PR: -

@rovere @felicepantaleo @bendavid @hatakeyamak (HGCal RECO and PF)

…into from-CMSSW_11_0_0_pre9

…kflow

…CL PF validation; and add a bit of documentation

…o be re-added later w/ modifier); avoid setting charge for neutrals

cmsbuild · 2019-10-04T16:02:43Z

The code-checks are being triggered in jenkins.

felicepantaleo · 2019-10-04T16:09:25Z

please test workflow 20493.52

cmsbuild · 2019-10-04T16:10:16Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28116/12139

This PR adds an extra 84KB to repository
Found files with invalid states:
- RecoHGCal/TICL/interface/TICLCandidateBuilderPlugins.h:
  - Added: 688ec64
  - Modified: 8406d8c, ab24ac4
  - Deleted: ecb6ad2

cmsbuild · 2019-10-04T16:10:44Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2788/console Started: 2019/10/04 18:11

cmsbuild · 2019-10-10T17:50:38Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0deee6/2875/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: 2961056
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2960714
DQMHistoTests: Total skipped: 341
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 147 log files, 16 edm output root files, 34 DQM output files

jfernan2 · 2019-10-11T06:52:05Z

+1

kpedro88 · 2019-10-14T15:13:13Z

+upgrade

rovere · 2019-10-15T14:33:09Z

@perrotta @slava77
A kind reminder.

perrotta · 2019-10-21T10:23:19Z

you are right @steggema , adding the top level BuildFile instructs scram to build a shared library RecoHGCalTICL and this cause scram to also test headers files using the dependency list of top level BuildFile. Please move the headers in plugins directory and drop top level BuildFile too. It is only needed when you want to build a public shared library

@steggema , I am belatedly late in jumping in this PR. Looking at the thread I see this comment from @smuzaffar which suggests to move all headers into the \plugin directory, and drop the top level BuildFile too.
As of now, I see that still TracksterMomentumPluginBase and TracksterTrackPluginBase sit in the \interface (and \src) directories, although being only used by the new plugins here: this prevents removing the top level Buildfile.

I see that this PR does not fail the headers check any more, and therefore it should be considered as technically fixed. However, I would like to get confirmation from @smuzaffar that he is fine with the current arrangements. or if it would still be preferable to move to \plugin whatever is not needed to build a public shared library.

smuzaffar · 2019-10-21T10:36:15Z

RecoHGCal/TICL/src/TracksterMomentumPluginBase.cc

+#include "RecoHGCal/TICL/interface/TracksterMomentumPluginBase.h"
+#include "FWCore/Framework/interface/MakerMacros.h"
+
+EDM_REGISTER_PLUGINFACTORY(TracksterMomentumPluginFactory, "TracksterMomentumPluginFactory");


this is wrong, we shoud never register a plugin from a shared library

@smuzaffar this is registering a factory, not a plugin. Doesn't this need to be in a shared library to be accessible?

ah did not realize that @kpedro88 . Looking at cmssw , yes plugin factories are ok to be in shared libs.

smuzaffar · 2019-10-21T10:36:20Z

RecoHGCal/TICL/src/TracksterTrackPluginBase.cc

+#include "RecoHGCal/TICL/interface/TracksterTrackPluginBase.h"
+#include "FWCore/Framework/interface/MakerMacros.h"
+
+EDM_REGISTER_PLUGINFACTORY(TracksterTrackPluginFactory, "TracksterTrackPluginFactory");


this is wrong, we shoud never register a plugin from a shared library

ignore the above comment

smuzaffar · 2019-10-21T10:38:45Z

@steggema and @perrotta , there is no need to have these interface/ src/ files. Currently these are registering edm plugins which will cause duplicate plugins if this library is linked by any other package.
Please move these to plugins/ and drop the top level BuildFile.

steggema · 2019-10-22T09:15:22Z

@perrotta From the updated responses, I understand that there's no action required from my side. Please let me know if I misunderstood.

rovere · 2019-10-22T11:47:13Z

From the last time I checked, pre11 is due today and it would be a pity if this Pr is not part of it.
Also, could someone confirm if this is still the confirmed schedule for CMSSW_11

perrotta · 2019-10-22T12:14:39Z

@perrotta From the updated responses, I understand that there's no action required from my side. Please let me know if I misunderstood.

Yes, as far as I understand @smuzaffar does not ask any more to move everything into the plugin directory and remove the top level Buildfile.
I just checked with him because that comment wasn't answered at the time.

smuzaffar · 2019-10-22T12:24:20Z

true, PR is its current state looks good. No need to move top level buildfile or interface, src files. headers file checks look good too.

perrotta · 2019-10-23T12:26:56Z

The footprint on CPU time and output size of this PR is quite limited, at least for the non PU workflow 20493.52

The addition to the AOD output is limited to the two new branches, with an overall contribution to the total size which is of the order of permil

-----------------------------------------------------------------
   or, B         new, B      delta, B   delta, %   deltaJ, %    branch 
-----------------------------------------------------------------
      0.0 ->       199.0        199     NEWO   0.06     recoPFCandidates_pfTICLProducer__RECO.
      0.0 ->       255.1        255     NEWO   0.07     TICLCandidates_ticlCandidateFromTrackstersProducer__RECO.
-------------------------------------------------------------
   351284 ->      351737        454             0.1     ALL BRANCHES

Nothing is added, for the moment, to the minAOD output
The CPU consumptions of the new modules is minimal:

  delta/mean delta/orJob     original                   new       module name
  ---------- ------------     --------                  ----       ------------
       added      +0.00%         0.00 ms/ev ->         0.02 ms/ev ticlPFValidation
       added      +0.00%         0.00 ms/ev ->         0.03 ms/ev pfTICLProducer
       added      +0.00%         0.00 ms/ev ->         0.02 ms/ev ticlCandidateFromTrackstersProducer
  ---------- ------------     --------                  ----       ------------
Job total:  1.00444 s/ev ==> 1.00693 s/ev

perrotta · 2019-10-23T12:30:45Z

+1

TICLCandidate's are created as intermediate step tpowards PF candidates
The implementation corresponds to what anticipated in the PR description, and also takes into account the following review
Jenkins tests show that "normal" Phase2 workflows are not affected, while for workflows in which TICL reconstruction is activated the expected products show up in output

cmsbuild · 2019-10-23T12:31:16Z

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-10-23T17:26:30Z

+1

steggema added 15 commits September 19, 2019 17:15

Merge commit 'refs/pull/27917/head' of https://github.com/cms-sw/cmssw …

a540139

…into from-CMSSW_11_0_0_pre9

Add first version of TICLCandidate

4d7c47b

Add initial versions of Trackster->TICLCandidate->PF plugins

688ec64

Working version of TICL -> PF

8406d8c

Add validation for PFCandidates produced via HGCal->TICL

d1f7844

Add option for trackster energy to be taken from regression

cd2e586

Filename consistency and other minor changes to complete TICL->PF wor…

ab24ac4

…kflow

Add track plugin to TICL candidate producer

ecb6ad2

Change track reference to edm::Ptr in TICLCandidate; add charge to TI…

e5b2ae7

…CL PF validation; and add a bit of documentation

Refactor loops in trackster->ticl candidate plugins, and code format

4d56107

Move validation from official sequences for now to ticl_iterations (t…

b0f323c

…o be re-added later w/ modifier); avoid setting charge for neutrals

Add minimum probability cut for tracksters to become TICLCandidates

458263b

Merge remote-tracking branch 'my-cmssw/TICLtoPF' into TICLtoPF

86e5fa1

Make particle ID probability lists and accessors consistent

1148b32

Address code checks

1fc2e24

cmsbuild added this to the CMSSW_11_0_X milestone Oct 4, 2019

cmsbuild added code-checks-pending comparison-pending dqm-pending orp-pending pending-signatures reconstruction-pending tests-pending upgrade-pending labels Oct 4, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 4, 2019

cmsbuild added the tests-approved label Oct 10, 2019

cmsbuild added comparison-available and removed comparison-pending labels Oct 10, 2019

cmsbuild added dqm-approved and removed dqm-pending labels Oct 11, 2019

cmsbuild added upgrade-approved and removed upgrade-pending labels Oct 14, 2019

smuzaffar reviewed Oct 21, 2019

View reviewed changes

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Oct 23, 2019

cmsbuild added orp-approved and removed orp-pending labels Oct 23, 2019

cmsbuild merged commit 5a7a185 into cms-sw:master Oct 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TICL: Create PF candidates as output of TICL #28116

TICL: Create PF candidates as output of TICL #28116

steggema commented Oct 4, 2019

cmsbuild commented Oct 4, 2019

felicepantaleo commented Oct 4, 2019

cmsbuild commented Oct 4, 2019

cmsbuild commented Oct 4, 2019 •

edited

cmsbuild commented Oct 10, 2019

jfernan2 commented Oct 11, 2019

kpedro88 commented Oct 14, 2019

rovere commented Oct 15, 2019

perrotta commented Oct 21, 2019

smuzaffar Oct 21, 2019

kpedro88 Oct 21, 2019

smuzaffar Oct 21, 2019

smuzaffar Oct 21, 2019

smuzaffar Oct 21, 2019

smuzaffar commented Oct 21, 2019

steggema commented Oct 22, 2019

rovere commented Oct 22, 2019

perrotta commented Oct 22, 2019

smuzaffar commented Oct 22, 2019 •

edited

perrotta commented Oct 23, 2019

perrotta commented Oct 23, 2019

cmsbuild commented Oct 23, 2019

fabiocos commented Oct 23, 2019

TICL: Create PF candidates as output of TICL #28116

TICL: Create PF candidates as output of TICL #28116

Conversation

steggema commented Oct 4, 2019

PR description:

PR validation:

if this PR is a backport please specify the original PR: -

cmsbuild commented Oct 4, 2019

felicepantaleo commented Oct 4, 2019

cmsbuild commented Oct 4, 2019

cmsbuild commented Oct 4, 2019 • edited

cmsbuild commented Oct 10, 2019

jfernan2 commented Oct 11, 2019

kpedro88 commented Oct 14, 2019

rovere commented Oct 15, 2019

perrotta commented Oct 21, 2019

smuzaffar Oct 21, 2019

Choose a reason for hiding this comment

kpedro88 Oct 21, 2019

Choose a reason for hiding this comment

smuzaffar Oct 21, 2019

Choose a reason for hiding this comment

smuzaffar Oct 21, 2019

Choose a reason for hiding this comment

smuzaffar Oct 21, 2019

Choose a reason for hiding this comment

smuzaffar commented Oct 21, 2019

steggema commented Oct 22, 2019

rovere commented Oct 22, 2019

perrotta commented Oct 22, 2019

smuzaffar commented Oct 22, 2019 • edited

perrotta commented Oct 23, 2019

perrotta commented Oct 23, 2019

cmsbuild commented Oct 23, 2019

fabiocos commented Oct 23, 2019

cmsbuild commented Oct 4, 2019 •

edited

smuzaffar commented Oct 22, 2019 •

edited