TICL version 4: CLUE3D as default Pattern Recognition, Linking #38880

waredjeb · 2022-07-28T11:13:40Z

This PR introduces TICLv4:

Using CLUE3D as default pattern recognition. Replaced the 4 Iterations with a single global one.
Enabling the possibility to fall back to TICLv3 with --procModifiers ticl_v3.
Tackster-Trackster and Trackster-Track geometrical linking. Propagating Tracksters and Tracks to
the same plane, if they fall in the same eta-phi window they are geometrically compatible. Trackster
and Tracks are then checked for Time compatibility. They are time compatible if within 3sigma of each other
or either of them does not have time available. Possible charged candidates are checked for energy compatibility
with the corresponding track. TICLCandidate and Track are energy compatible if the TICLCandidate's tracksters
cumulative energy does not exceed track momentum by more than a threshold. Track with no matched Trackster are
promoted to charged-TICLCandidate. Note that muon tracks are masked and not used for the linking.
Improved Particle Flow interpretation
More details on the Linking step here: Linking
CLUE3D performance compared to Cellular Automaton:
- CLUE3DvsCA
- CLUE3DvsCATiming

PR validated on CMSSW_12_5_0_pre3.

Timing comparison between TICLv4 and TICLv3, obtained on RelVal QCD PU 200, 10 events:
- TICLv4
- TICLv3

Physics Performances:

RecoParticleFlow Validation on RelValQCD_Pt15To7000_Flat_14 PU200, D88 Geometry:
- PFValidation

EDIT 08/08 :

Small bug fix, setting Trackster seedIndex default value to -1, since 0 is a valid seedIndex.

cmsbuild · 2022-07-28T11:22:01Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38880/31308

ERROR: Build errors found during clang-tidy run.

RecoHGCal/TICL/plugins/LinkingAlgoByDirectionGeometric.cc:91:12: error: variable-sized object may not be initialized [clang-diagnostic-error]
  int mask[trackstersSize] = {0};
           ^
RecoHGCal/TICL/plugins/LinkingAlgoByDirectionGeometric.cc:329:19: error: variable-sized object may not be initialized [clang-diagnostic-error]
  int chargedMask[tracksters.size()] = {0};
                  ^
RecoHGCal/TICL/plugins/LinkingAlgoByDirectionGeometric.cc:458:9: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
--
RecoHGCal/TICL/plugins/LinkingAlgoByDirectionGeometric.cc:476:19: error: variable-sized object may not be initialized [clang-diagnostic-error]
  int neutralMask[tracksters.size()] = {0};
                  ^
RecoHGCal/TICL/plugins/LinkingAlgoByDirectionGeometric.cc:519:11: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
--
gmake: *** [config/SCRAM/GMake/Makefile.coderules:129: code-checks] Error 2
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

…ck Linking This commit is a set of multiple commits: - Using CLUE3D as default pattern recognition. Replaced the 4 Iterations with a single global one. Enabling possibility to fall back to TICLv3 with `--procModifier ticl_v3`. - Tackster-Trackster and Trackster-Track geometrical linking. Propagating Tracksters and Tracks to the same plane, if they fall in the same eta-phi window they are geometrically compatible. Trackster and Tracks are then checked for Time compatibility. They are time compatible if within 3sigma of each other or either of them does not have time available. Possible charged candidates are checked for energy compatibility with the corresponding track. TICLCandidate and Track are energy compatible if the TICLCandidate's tracksters cumulative energy does not exceed track momentum by more than a threshold. Track with no matched Trackster are promoted at charged-TICLCandidate. Note that muon tracks are masked and not used for the linking. Improving Particle Flow interpreation. Co-Authored-By: Abhirikshma Nandi <[email protected]> Co-Authored-By: Marco Rovere <[email protected]> Co-Authored-By: Felice Pantaleo <[email protected]>

cmsbuild · 2022-07-28T12:59:34Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38880/31311

This PR adds an extra 68KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File Configuration/PyReleaseValidation/python/upgradeWorkflowComponents.py modified in PR(s): Update BeamSpot in Run3 MC GTs + swap Run3RoundOptics25ns13TeVLowSigmaZ with Realistic25ns13p6TeVEarly2022Collision vtx smearing #38760, Add NANO step and Mini+Nano validations to FastSim workflows #38834, Clean up Phase-2 geometry (Up to D85) #38702, Add run3 tracking low pu era #33532

cmsbuild · 2022-07-28T13:00:00Z

A new Pull Request was created by @waredjeb (Wahid Redjeb) for master.

It involves the following packages:

Configuration/ProcessModifiers (operations)
Configuration/PyReleaseValidation (pdmv, upgrade)
RecoHGCal/TICL (upgrade, reconstruction)
Validation/HGCalValidation (dqm)

@perrotta, @rappoccio, @jordan-martins, @bbilin, @pmandrik, @emanueleusai, @ahmad3213, @cmsbuild, @AdrianoDee, @srimanob, @jfernan2, @clacaputo, @kskovpen, @jpata, @qliphy, @rvenditti, @micsucmed, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@felicepantaleo, @forthommel, @kpedro88, @Martin-Grunewald, @bsunanda, @pfs, @trtomei, @slomeo, @youyingli, @sethzenz, @apsallid, @makortel, @lgray, @missirol, @beaucero, @vandreev11, @rovere, @cseez, @hatakeyamak, @ebrondol, @fabiocos, @sobhatta, @lecriste this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

rovere · 2022-07-28T13:09:35Z

@cmsbuild please test

cmsbuild · 2022-07-28T17:22:32Z

-1

Failed Tests: RelVals
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-51a7ed/26506/summary.html
COMMIT: ccabda0
CMSSW: CMSSW_12_5_X_2022-07-28-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38880/26506/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

39434.75

----- Begin Fatal Exception 28-Jul-2022 16:15:41 CEST-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Validating configuration of module: class=TrackstersMergeProducer label='ticlTrackstersMerge'
Exception Message:
Illegal parameters found in configuration.  The parameters are named:
 'seedingTrk'
 'trackstersem'
 'trackstershad'
 'tracksterstrk'
 'tracksterstrkem'
You could be trying to use parameter names that are not
allowed for this plugin or they could be misspelled.
----- End Fatal Exception -------------------------------------------------

jpata · 2022-08-02T11:04:27Z

RecoHGCal/TICL/plugins/LinkingAlgoByDirectionGeometric.cc

+      double eta_min = std::max(abs(seed_eta) - delta, (double)TileConstants::minEta);
+      double eta_max = std::min(abs(seed_eta) + delta, (double)TileConstants::maxEta);
+
+      std::array<int, 4> search_box = tile.searchBoxEtaPhi(eta_min, eta_max, seed_phi - delta, seed_phi + delta);
+      if (search_box[2] > search_box[3])
+        search_box[3] += TileConstants::nPhiBins;
+
+      for (int eta_i = search_box[0]; eta_i <= search_box[1]; ++eta_i) {
+        for (int phi_i = search_box[2]; phi_i <= search_box[3]; ++phi_i) {
+          const auto &in_box = tile[tile.globalBin(eta_i, (phi_i % TileConstants::nPhiBins))];
+          for (const unsigned t_i : in_box) {
+            if (!mask[t_i]) {
+              resultCollection[seedId].push_back(t_i);
+              if (useMask)
+                mask[t_i] = 1;
+            }
+          }
+        }
+      }


this seems identical to the branch above. any chance you can define it as a common function to avoid code duplication?

thanks for the comments. Sure, I can avoid code duplication

jpata

Some general code comments:

avoid hardcoding magic numbers, this makes the code hard to maintain in the future
try to reduce copy-paste parts if possible (e.g. when making particle candidates)
it might be good to use a single logging configuration across CMS. Do you really need to introduce another runtime logging level on top of the EDM MessageLogger?
vectors are often not preallocated. did you confirm that it's never possible?

Can you also provide some representative profiling (CPU and memory) information of the new algo?

jpata · 2022-08-02T11:06:21Z

RecoHGCal/TICL/plugins/LinkingAlgoByDirectionGeometric.cc

+
+void LinkingAlgoByDirectionGeometric::dumpLinksFound(std::vector<std::vector<unsigned>> &resultCollection,
+                                                     const char *label) const {
+  if (!(LinkingAlgoBase::algo_verbosity_ > LinkingAlgoBase::Advanced))


you could also wrap the contents of this function in #ifdef EDM_ML_DEBUG so that it's compiled if and only if debug is enabled.
it may be confusing for non-TICL people in the future to navigate multiple parallel levels of message logging.

now the function implementation is not defined if EDM_ML_DEBUG is not defined. In fact I'm confused why scram does not crash at linking due to a missing implementation.

I thought something like this might be desirable here:

void LinkingAlgoByDirectionGeometric::dumpLinksFound(std::vector<std::vector<unsigned>> &resultCollection, const char *label) const { #ifdef EDM_ML_DEBUG ... #endif }

jpata · 2022-08-02T11:07:56Z

RecoHGCal/TICL/plugins/LinkingAlgoByDirectionGeometric.cc

+                                                     const std::vector<reco::Muon> &muons,
+                                                     const edm::Handle<std::vector<Trackster>> tsH,
+                                                     std::vector<TICLCandidate> &resultLinked) {
+  constexpr double mpion = 0.13957;


it might be good to define the TICL pi mass in a constexpr somewhere, also replacing

RecoHGCal/TICL/plugins/TICLCandidateFromTrackstersProducer.cc: constexpr double mpion2 = 0.13957 * 0.13957; RecoHGCal/TICL/plugins/TracksterP4FromTrackAndPCA.cc: constexpr double mpion2 = 0.13957 * 0.13957; RecoHGCal/TICL/plugins/TrackstersMergeProducer.cc: constexpr double mpion = 0.13957;

jpata · 2022-08-02T11:09:41Z

RecoHGCal/TICL/plugins/LinkingAlgoByDirectionGeometric.cc

+    reco::TrackRef trackref = reco::TrackRef(tkH, i);
+    // also veto tracks associated to muons
+    int muId = PFMuonAlgo::muAssocToTrack(trackref, muons);
+    if (LinkingAlgoBase::algo_verbosity_ > LinkingAlgoBase::Advanced)


it seems strange to protect a compile-time-enabled message with a runtime flag.

Hi! We use this strategy to have different log-debug levels in TICL, but if this is a problem I can remove it

jpata · 2022-08-02T11:10:49Z

RecoHGCal/TICL/plugins/LinkingAlgoByDirectionGeometric.cc

+    candidateTrackIds.push_back(i);
+
+    // don't consider tracks below 2 GeV for linking
+    if (std::sqrt(tk.p() * tk.p() + mpion2) < 2.0)


it might useful if this 2 GeV cut was a constexpr somewhere in the class configuration.

jpata · 2022-08-02T11:11:30Z

RecoHGCal/TICL/plugins/LinkingAlgoByDirectionGeometric.cc

+          << t.time() << " energy " << t.raw_energy() << "\n";
+    Vector directnv = t.eigenvectors(0);
+
+    if (abs(directnv.Z()) < 0.00001)


this magic number should be a constexpr.

jpata · 2022-08-02T11:17:58Z

RecoHGCal/TICL/plugins/LinkingAlgoByDirectionGeometric.h

+#include "Geometry/Records/interface/IdealGeometryRecord.h"
+#include "Geometry/CommonDetUnit/interface/GeomDet.h"
+
+//#include "RecoHGCal/TICL/test/PCA_mod.h"


jpata · 2022-08-02T11:20:09Z

RecoHGCal/TICL/plugins/TrackstersMergeProducer.cc

+  edm::Handle<std::vector<reco::Muon>> muons_h;
+  evt.getByToken(muons_token_, muons_h);
+  const auto &muons = *muons_h;


Suggested change

edm::Handle<std::vector<reco::Muon>> muons_h;

evt.getByToken(muons_token_, muons_h);

const auto &muons = *muons_h;

const auto& muons = evt.get(muons_token_);

also below

jpata · 2022-08-02T11:21:18Z

RecoHGCal/TICL/plugins/TrackstersMergeProducer.cc

-    tmpCandidate.setP4(p4);
-    resultCandidates->push_back(tmpCandidate);
+  // Print debug info
+  if (debug_) {


same comment as above about the necessity of additional runtime flags in front of compile-time-enabled logging.

jpata · 2022-08-02T11:24:06Z

RecoHGCal/TICL/plugins/TrackstersMergeProducerV3.cc

+  }
+
+  // Neutral Hadrons
+  constexpr double mpion = 0.13957;


jpata · 2022-08-02T11:25:19Z

RecoHGCal/TICL/plugins/TrackstersMergeProducerV3.cc

+            tmpCandidate.setCharge(track.charge());
+            tmpCandidate.setTrackPtr(edm::Ptr<reco::Track>(track_h, trackIdx));
+            tmpCandidate.setPdgId(211 * track.charge());
+            for (auto otherTracksterIdx : trackstersTRKwithSameSeed) {
+              tmpCandidate.addTrackster(edm::Ptr<ticl::Trackster>(trackstersMergedHandle, otherTracksterIdx));
+              raw_energy += trackstersMergedHandle->at(otherTracksterIdx).raw_energy();
+            }
+            tmpCandidate.setRawEnergy(raw_energy);
+            math::XYZTLorentzVector p4(raw_energy * track.momentum().unit().x(),
+                                       raw_energy * track.momentum().unit().y(),
+                                       raw_energy * track.momentum().unit().z(),
+                                       raw_energy);
+            tmpCandidate.setP4(p4);
+            resultCandidates->push_back(tmpCandidate);


consider reducing the repetition with respect to the code below by using a function

I think in this case a function would make the code readability harder, I will rearrange the code a bit to avoid duplication as much as possible, but I think it would be better to have all the logic to build the candidates more explicit.

if (foundCompatibleTRK) { resultCandidates->push_back(buildChargedHadron(...)); } } else { resultCandidates->push_back(buildElectron(...)); } ...

I would suggest something like this here and below. Having code blocks that are hundreds of lines long seems difficult to read and maintain later.

Hello @jpata, thanks for the comments! I think implementing functions is slightly unnecessary here:

this is part of the TICL v3 logic which is deprecated and will be removed soon (and indeed this code is already in production as TrackstersMergeProducer.cc, we just changed the name of the file to keep it there till everything switches to using TICL v4)

It would require some non-trivial modification of the code, and we risk introducing bugs in something which is already very well tested

srimanob · 2022-08-26T11:29:00Z

+Upgrade

Move TICL to v4, and introduce the workflow to roll back to v3.

kskovpen · 2022-08-26T11:32:33Z

+pdmv

rovere · 2022-08-29T07:33:52Z

Would it be possible to have the remaining signatures added in the coming days? It would be nice to have this PR integrated asap.

emanueleusai · 2022-08-29T18:41:15Z

+1

rappoccio · 2022-08-30T15:59:29Z

+1

cmsbuild · 2022-08-30T15:59:55Z

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 be automatically merged.

cmsbuild added this to the CMSSW_12_5_X milestone Jul 28, 2022

cmsbuild added code-checks-pending dqm-pending operations-pending orp-pending pdmv-pending pending-signatures reconstruction-pending tests-pending upgrade-pending labels Jul 28, 2022

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jul 28, 2022

waredjeb force-pushed the ticl_v4_clue3d_CMSSW_12_5_0_pre3 branch from 5b4ab78 to ccabda0 Compare July 28, 2022 12:52

cmsbuild added code-checks-pending and removed code-checks-rejected labels Jul 28, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 28, 2022

cmsbuild added tests-started and removed tests-pending labels Jul 28, 2022

cmsbuild added tests-rejected and removed tests-started labels Jul 28, 2022

cmsbuild mentioned this pull request Jul 31, 2022

Migration of input collection parameters to untracked for CTPPS and DTMonitorModule #38854

Closed

jpata reviewed Aug 2, 2022

View reviewed changes

cmsbuild added the tests-approved label Aug 26, 2022

cmsbuild added upgrade-approved and removed upgrade-pending labels Aug 26, 2022

cmsbuild added pdmv-approved and removed pdmv-pending labels Aug 26, 2022

This was referenced Aug 26, 2022

Adding V0 Validation Wfs to the RelVal Matrix #39215

Merged

ECAL - Implement follow up issues for Phase 2 GPU reconstruction #39195

Merged

smuzaffar modified the milestones: CMSSW_12_5_X, CMSSW_12_6_X Aug 28, 2022

cmsbuild added dqm-approved and removed dqm-pending labels Aug 29, 2022

rappoccio merged commit 92ac7dc into cms-sw:master Aug 30, 2022

cmsbuild added fully-signed operations-approved orp-approved and removed operations-pending pending-signatures orp-pending labels Aug 30, 2022

clacaputo mentioned this pull request Sep 22, 2022

[12_6_0_pre2] Reco timing increase in Phase2 reconstruction #39482

Closed

swagata87 mentioned this pull request Oct 21, 2022

[EGM HLT@Phase2] Move to TICL v4 #39820

Merged

TICL version 4: CLUE3D as default Pattern Recognition, Linking #38880

TICL version 4: CLUE3D as default Pattern Recognition, Linking #38880

Uh oh!

Conversation

waredjeb commented Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmsbuild commented Jul 28, 2022

Uh oh!

cmsbuild commented Jul 28, 2022

Uh oh!

cmsbuild commented Jul 28, 2022

Uh oh!

rovere commented Jul 28, 2022

Uh oh!

cmsbuild commented Jul 28, 2022

RelVals

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpata left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srimanob commented Aug 26, 2022

Uh oh!

kskovpen commented Aug 26, 2022

Uh oh!

rovere commented Aug 29, 2022

Uh oh!

emanueleusai commented Aug 29, 2022

Uh oh!

rappoccio commented Aug 30, 2022

Uh oh!

cmsbuild commented Aug 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

waredjeb commented Jul 28, 2022 •

edited

Loading