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.

Sign up for GitHub

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

Jump to bottom

Introduce support for cluster splitting and JetCore iteration in Phase-2 track reconstruction #46001

Merged
merged 4 commits into from
Oct 23, 2024
Merged

Introduce support for cluster splitting and JetCore iteration in Phase-2 track reconstruction #46001

merged 4 commits into from
Oct 23, 2024

Conversation

langufo
Copy link

@langufo langufo commented Sep 13, 2024

PR description:

This PR introduces the support for cluster splitting and a JetCore iteration in the Phase-2 track reconstruction, while keeping both of them disabled by default.

The effect of this PR on the Phase-1 and Phase-2 reconstruction (resource utilization & tracking performance) are summarized in this presentation at the Tracking POG meeting: https://indico.cern.ch/event/1428996/#38-integration-of-jetcore-for
Information about the tuning of the cluster splitting parameters for Phase-2 in this other presentation: https://indico.cern.ch/event/1396796/#30-jetcore-and-cluster-splitti

Some details about this PR:

  • The JetCoreClusterSplitter in RecoLocalTracker/SubCollectionProducers has been modified to support the features of the Phase-2 Inner Tracker (geometry and 3D sensors in TBPX layer 1). No worsening of the tracking performance in the Phase-1 reconstruction was observed following these changes (validation plots here).
  • Two process modifiers have been introduced:
    • splitClustersInPhase2Pixel, enabling an InitialStepPreSplitting to produce a collection of split clusters for use in the InitialStep;
    • jetCoreInPhase2, enabling the JetCore iteration in the Phase-2 Tracking.
  • Two workflow offsets have been added:
    • 0.19001 activates the splitClustersInPhase2Pixel process modifier;
    • 0.19002 activates both the splitClustersInPhase2Pixel and the jetCoreInPhase2 modifiers.

PR validation:

  • Compiles fine after git cms-checkdeps -a -A.
  • export CMS_PATH=/cvmfs/cms-ib.cern.ch/ ; export SITECONFIG_PATH=/cvmfs/cms-ib.cern.ch/SITECONF/local/ ; scram b runtests: no failing test.
  • runTheMatrix.py -l limited -i all --ibeos: no failing test.
  • Tested with step 3 on RelVals from DAS (TTBar_14TeV + PU and QCD_FlatPt_15_3000HS_14 without PU), runs to completion.

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

This PR is not a backport, and no backport is needed.

@cmsbuild cmsbuild added this to the CMSSW_14_2_X milestone Sep 13, 2024
@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 13, 2024 •
edited
Loading

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46001/41774

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • Configuration/ProcessModifiers (operations)
  • Configuration/PyReleaseValidation (upgrade, pdmv)
  • RecoLocalTracker/SubCollectionProducers (reconstruction)
  • RecoTracker/FinalTrackSelectors (reconstruction)
  • RecoTracker/IterativeTracking (reconstruction)

@AdrianoDee, @antoniovilela, @cmsbuild, @davidlange6, @fabiocos, @jfernan2, @kskovpen, @mandrenguyen, @miquork, @rappoccio, @srimanob, @subirsarkar, @sunilUIET can you please review it and eventually sign? Thanks.
@GiacomoSguazzoni, @JanFSchulte, @Martin-Grunewald, @VinInn, @VourMa, @alesaggio, @dgulhan, @echabert, @fabiocos, @felicepantaleo, @gbenelli, @gpetruc, @jlidrych, @makortel, @missirol, @mmusich, @mtosi, @robervalwalsh, @rovere, @slomeo, @threus, @yduhm this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

mmusich
mmusich reviewed Sep 13, 2024
View reviewed changes
Configuration/ProcessModifiers/python/jetCoreInPhase2_cff.py Outdated
@@ -0,0 +1,4 @@
import FWCore.ParameterSet.Config as cms

from Configuration.ProcessModifiers.splitClustersInPhase2Pixel_cff import splitClustersInPhase2Pixel
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need this?

Copy link
Author

Choose a reason for hiding this comment

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

I missed it while cleaning up. Line 3 now deleted.

Configuration/PyReleaseValidation/python/upgradeWorkflowComponents.py Outdated
Comment on lines 2449 to 2453
#'Reco',
#'RecoFakeHLT',
#'RecoGlobal',
#'RecoNano',
#'RecoNanoFakeHLT',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#'Reco',
#'RecoFakeHLT',
#'RecoGlobal',
#'RecoNano',
#'RecoNanoFakeHLT',

remove?

RecoLocalTracker/SubCollectionProducers/src/JetCoreClusterSplitter.cc
@@ -111,6 +132,10 @@ void JetCoreClusterSplitter::produce(edm::Event& iEvent, const edm::EventSetup&
edmNew::DetSetVector<SiPixelCluster>::FastFiller filler(*output, detIt->id());
const edmNew::DetSet<SiPixelCluster>& detset = *detIt;
const GeomDet* det = geometry->idToDet(detset.id());
float tanLorentzAngle = tanLorentzAngle_;
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this come from the database?

RecoLocalTracker/SubCollectionProducers/src/JetCoreClusterSplitter.cc
@@ -111,6 +132,10 @@ void JetCoreClusterSplitter::produce(edm::Event& iEvent, const edm::EventSetup&
edmNew::DetSetVector<SiPixelCluster>::FastFiller filler(*output, detIt->id());
const edmNew::DetSet<SiPixelCluster>& detset = *detIt;
const GeomDet* det = geometry->idToDet(detset.id());
float tanLorentzAngle = tanLorentzAngle_;
if (DetId(detset.id()).subdetId() == 1 /* px barrel */ && topology->pxbLayer(detset.id()) == 1) {
tanLorentzAngle = tanLorentzAngleBarrelLayer1_;
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this come from the database?

Copy link
Author

Choose a reason for hiding this comment

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

These parameters were originally hardcoded. I gave them the names they now have to try to interpret them within a simple geometrical model, but I'm not sure how close they are to the quantity stored by the same name in the DB, as for Phase-1 I've tried to preserve to values already in place.

As the tracking performance seems to be not too sensitive to the fine-tuning of these parameters (from what I could see), if possible I would keep this implementation as it currently is for this PR, and study the effect of using the parameters from the DB as a future development.

Copy link
Contributor

Choose a reason for hiding this comment

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

As the tracking performance seems to be not too sensitive to the fine-tuning of these parameters (from what I could see), if possible I would keep this implementation as it currently is for this PR, and study the effect of using the parameters from the DB as a future development.

I think it would be nice to study the performance doing a scan of the values around the current parametrization. It might even be that we have sub-par performance with the current Run-3 reconstruction because of that.

RecoLocalTracker/SubCollectionProducers/src/JetCoreClusterSplitter.cc
if (expSizeY < 1.f)
expSizeY = 1.f;
float expSizeX = 1.5f;
if (isEndCap) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why the endcap special treatment is not applied anymore ?

Copy link
Author

Choose a reason for hiding this comment

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

In general, the treatment that was applied in the case of a module from the endcaps should now be covered by computing everything in the local frame of the module.

However, it is also true that I initially forgot to update the definition of jetZOverRho, such that its value was wrong in the endcaps. I've now fixed this, and I've rerun a step3 + validation to check that everything it's still ok.

(As a general comment, the value of jetZOverRho in the endcaps now is slightly different from upstream CMSSW, due to the turbine-like geometry that previously was not not taken into account.)

RecoLocalTracker/SubCollectionProducers/src/JetCoreClusterSplitter.cc Outdated
Comment on lines 91 to 92
pitchX_(iConfig.getParameter<double>("pitchX")),
pitchY_(iConfig.getParameter<double>("pitchY")),
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this come from the topology?

Copy link
Author

Choose a reason for hiding this comment

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

Modified as proposed.

RecoLocalTracker/SubCollectionProducers/src/JetCoreClusterSplitter.cc Outdated
tanLorentzAngleBarrelLayer1_(iConfig.getParameter<double>("tanLorentzAngleBarrelLayer1")),
pitchX_(iConfig.getParameter<double>("pitchX")),
pitchY_(iConfig.getParameter<double>("pitchY")),
thickness_(iConfig.getParameter<double>("thickness")),
Copy link
Contributor

Choose a reason for hiding this comment

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

here as well.

Copy link
Author

Choose a reason for hiding this comment

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

Modified as proposed.

RecoTracker/IterativeTracking/python/JetCoreRegionalStep_cff.py Outdated
Comment on lines 363 to 379
#useAnyMVA = cms.bool(False),
#GBRForestLabel = cms.string('MVASelectorIter0'),
#trackSelectors = [
# RecoTracker.FinalTrackSelectors.multiTrackSelector_cfi.looseMTS.clone(
# name = 'jetCoreRegionalStepLoose',
# ), #end of pset
# RecoTracker.FinalTrackSelectors.multiTrackSelector_cfi.tightMTS.clone(
# name = 'jetCoreRegionalStepTight',
# preFilterName = 'jetCoreRegionalStepLoose',
# ),
# RecoTracker.FinalTrackSelectors.multiTrackSelector_cfi.highpurityMTS.clone(
# name = 'QualityMasks',
# preFilterName = 'jetCoreRegionalStepTight',
# ),
#],
#useAnyMVA = None,
#GBRForestLabel = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to keep?

Copy link
Author

Choose a reason for hiding this comment

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

Commented lines deleted.

@cmsbuild cmsbuild mentioned this pull request Sep 13, 2024
Merged
3 tasks
@langufo langufo force-pushed the ph2-pixel-cluster-splitting-and-jetcore branch from fdc1930 to 4ad7e95 Compare September 16, 2024 09:23
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46001/41802

@cmsbuild
Copy link
Contributor

Pull request #46001 was updated. @AdrianoDee, @antoniovilela, @cmsbuild, @davidlange6, @fabiocos, @jfernan2, @kskovpen, @mandrenguyen, @miquork, @rappoccio, @srimanob, @subirsarkar, @sunilUIET can you please check and sign again.

@mmusich
Copy link
Contributor

mmusich commented Sep 16, 2024

test parameters:

  • relvals_opt = --what upgrade
  • workflows = 29624.19001, 29624.19002

@mmusich
Copy link
Contributor

mmusich commented Sep 16, 2024

@cmsbuild, please test

@mmusich
Copy link
Contributor

mmusich commented Sep 16, 2024 •
edited
Loading

@langufo the changes proposed here need to be propagated in the HLT menu.
See addOnTests failures

----- Begin Fatal Exception 16-Sep-2024 14:22:18 CEST-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing module: class=JetCoreClusterSplitter label='hltSiPixelClustersAfterSplittingPPOnAA'
Exception Message:
MissingParameter: Parameter 'expSizeXAtLorentzAngleIncidence' not found.
----- End Fatal Exception -------------------------------------------------

as this parameter is new, it's not already present in the current HLT tables.
I would suggest to add a fillDescriptions method for JetCoreClusterSplitter such that it takes care of the update automatically with a default (see this twiki for details).

This was referenced Sep 19, 2024
Merged
Merged
Merged
@cmsbuild cmsbuild mentioned this pull request Sep 27, 2024
Merged
@srimanob
Copy link
Contributor

@cmsbuild please test

@srimanob
Copy link
Contributor

Ping @cms-sw/pdmv-l2 @cms-sw/upgrade-l2

@Moanwar
Copy link
Contributor

Moanwar commented Oct 14, 2024

+Upgrade

@AdrianoDee
Copy link
Contributor

+pdmv

@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 12KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0b2256/42158/summary.html
COMMIT: 551a836
CMSSW: CMSSW_14_2_X_2024-10-14-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46001/42158/install.sh to create a dev area with all the needed externals and cmssw changes.

  • DAS Queries: The DAS query tests failed, see the summary page for details.

Comparison Summary

Summary:

This was referenced Oct 18, 2024
Merged
Merged
@mandrenguyen
Copy link
Contributor

+1

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

@cmsbuild cmsbuild merged commit 4635bff into cms-sw:master Oct 23, 2024
12 checks passed
@cmsbuild cmsbuild mentioned this pull request Oct 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmusich mmusich mmusich left review comments

Assignees
No one assigned
Labels
code-checks-approved fully-signed operations-approved orp-approved pdmv-approved reconstruction-approved tests-approved tracking trk upgrade-approved
Projects
None yet
Milestone
CMSSW_14_2_X
Development

Successfully merging this pull request may close these issues.
8 participants
@langufo @cmsbuild @mmusich @jfernan2 @srimanob @Moanwar @AdrianoDee @mandrenguyen