Adapt pixel cpe algo to better handle broken clusters [15.0.X] by mroguljic · Pull Request #48008 · cms-sw/cmssw

mroguljic · 2025-05-05T09:06:12Z

Backport
Backport of #47966 to CMSSW 15.0.X to request data rereco for validation

PR description:

At the end of 2024, cluster breakage at high eta became significant. This caused issues when trying to improve calibrations for pixel cluster (position) parameter estimation, CPE.

This fix changes the CPE algorithms by relying on the "good" cluster edge, instead of both edges. This alternative algorithm is only used if the clusters are shorter than expected (e.g. broken). The change affects template reconstruction, and generic reconstruction both at CPU and GPU (alpaka). The fix is gated behind process modifiers for testing. The new versions of CPE algorithms require corresponding condition updates.

PR validation:

Undergoing runTheMatrix.py -l limited -i all --ibeos. We don't expect any workflow to be affect since the changes are protected by process modifiers. The PR was opened before the validation to allower others to comment early.

cmsbuild · 2025-05-05T09:06:35Z

A new Pull Request was created by @mroguljic for CMSSW_15_0_X.

It involves the following packages:

CondFormats/SiPixelTransient (db, reconstruction)
Configuration/ProcessModifiers (operations)
RecoLocalTracker/SiPixelRecHits (reconstruction)

@antoniovilela, @atpathak, @cmsbuild, @consuegs, @davidlange6, @fabiocos, @francescobrivio, @jfernan2, @mandrenguyen, @perrotta, @rappoccio can you please review it and eventually sign? Thanks.
@GiacomoSguazzoni, @JanChyczynski, @Martin-Grunewald, @PonIlya, @VinInn, @VourMa, @dkotlins, @fabiocos, @felicepantaleo, @ferencek, @gpetruc, @makortel, @missirol, @mmusich, @mroguljic, @mtosi, @rovere, @rsreds, @seemasharmafnal, @threus, @tsusa, @tvami, @yuanchao 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

Backported from Adapt pixel cpe algo to better handle broken clusters #47966

cmsbuild · 2025-05-05T09:06:35Z

cms-bot internal usage

mmusich

some naive questions / comments from a non-reviewer.

mmusich · 2025-05-05T09:37:44Z

assign heterogeneous

for the pixel local reco on GPU part

cmsbuild · 2025-05-05T09:38:04Z

New categories assigned: heterogeneous

@fwyzard,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks

mmusich · 2025-05-05T10:16:07Z

backport of #47966

mmusich · 2025-05-05T10:16:18Z

enable gpu

mroguljic · 2025-05-05T10:44:11Z

runTheMatrix finished with: 50 48 24 19 14 1 1 1 1 1 1 tests passed, 0 1 22 1 0 0 0 0 0 0 0 failed
I'm going through the logs to see what the issues are.

cmsbuild · 2025-05-06T05:55:52Z

Pull request #48008 was updated. @antoniovilela, @atpathak, @cmsbuild, @consuegs, @davidlange6, @fabiocos, @francescobrivio, @fwyzard, @jfernan2, @makortel, @mandrenguyen, @perrotta, @rappoccio can you please check and sign again.

mroguljic · 2025-05-06T05:59:52Z

runTheMatrix finished with 50 49 46 41 19 1 1 1 1 1 1 tests passed, 0 0 1 0 0 0 0 0 0 0 0 failed. Workflow 1000.0 fails at step3 with

----- Begin Fatal Exception 06-May-2025 07:46:12 CEST-----------------------
An exception of category 'FatalRootError' occurred while
   [0] Calling EventProcessor::runToCompletion (which does almost everything after beginJob and before endJob)
   Additional Info:
      [a] Fatal Root Error: @SUB=TStorageFactorySystem::Unlink
Unsupported

----- End Fatal Exception -------------------------------------------------

Not sure if this is related to the changes.

perrotta

Review and code development should be done in the development (master) release, and only backported when stabilized. However, since some review already started in ths backport PR, let me add a few comments here.
It would be preferable, however, that since now the two PRs get synchronized, and then the review continue in the master.

perrotta · 2025-05-06T06:26:39Z

+    // define the centers of the first last last pixel coordinates
+    float x1 = pitch * (upper_edge_first_pix - 0.5 * pitchfraction_first);
+    float x2 = pitch * (lower_edge_last_pix + 0.5 * pitchfraction_last);
+
+    if (delta / pitch > delta_length_cut) {
+      //  observed cluster is much shorter than expected, use one-sided reco
+      if (w_pred > 0.f) {


Suggested change

// define the centers of the first last last pixel coordinates

float x1 = pitch * (upper_edge_first_pix - 0.5 * pitchfraction_first);

float x2 = pitch * (lower_edge_last_pix + 0.5 * pitchfraction_last);

if (delta / pitch > delta_length_cut) {

// observed cluster is much shorter than expected, use one-sided reco

if (w_pred > 0.f) {

if (delta / pitch > delta_length_cut) {

// define the centers of the first and last pixel coordinates

float x1 = pitch * (upper_edge_first_pix - 0.5 * pitchfraction_first);

float x2 = pitch * (lower_edge_last_pix + 0.5 * pitchfraction_last);

// observed cluster is much shorter than expected, use one-sided reco

if (w_pred > 0.f) {

perrotta · 2025-05-06T06:28:58Z


    edm::ParameterSet pset_;
    bool useErrorsFromTemplates_;
+    bool GoodEdgeAlgo_;


Naming rule nr 9 in https://cms-sw.github.io/cms_coding_rules.html

Suggested change

bool GoodEdgeAlgo_;

bool goodEdgeAlgo_;

perrotta · 2025-05-06T06:29:29Z

    auto const& myname = p.getParameter<std::string>("ComponentName");
    auto const& magname = p.getParameter<edm::ESInputTag>("MagneticFieldRecord");
    useErrorsFromTemplates_ = p.getParameter<bool>("UseErrorsFromTemplates");
+    GoodEdgeAlgo_ = p.getParameter<bool>("GoodEdgeAlgo");


Suggested change

GoodEdgeAlgo_ = p.getParameter<bool>("GoodEdgeAlgo");

goodEdgeAlgo_ = p.getParameter<bool>("GoodEdgeAlgo");

perrotta · 2025-05-06T06:30:01Z

                                                                   genErrorDBObjectProduct,
-                                                                   lorentzAngleWidthProduct);
+                                                                   lorentzAngleWidthProduct,
+                                                                   GoodEdgeAlgo_);


Suggested change

GoodEdgeAlgo_);

goodEdgeAlgo_);

perrotta · 2025-05-06T06:32:26Z

                                                              const SiPixelGenErrorDBObject* genErrorDBObject,
-                                                              const SiPixelLorentzAngle* lorentzAngleWidth)
+                                                              const SiPixelLorentzAngle* lorentzAngleWidth,
+                                                              const bool GoodEdgeAlgo)


Suggested change

const bool GoodEdgeAlgo)

const bool goodEdgeAlgo)

perrotta · 2025-05-06T06:35:21Z

  cout << "(int)useErrorsFromTemplates_ = " << (int)useErrorsFromTemplates_ << endl;
  cout << "truncatePixelCharge_         = " << (int)truncatePixelCharge_ << endl;
  cout << "IrradiationBiasCorrection_   = " << (int)IrradiationBiasCorrection_ << endl;
+  cout << "GoodEdgeAlgo_                = " << (int)GoodEdgeAlgo_ << endl;


Suggested change

cout << "GoodEdgeAlgo_ = " << (int)GoodEdgeAlgo_ << endl;

cout << "goodEdgeAlgo_ = " << (int)GoodEdgeAlgo_ << endl;

perrotta · 2025-05-06T06:35:31Z

-      the_size_cutX);  // cut for eff charge width &&&
+      the_size_cutX,  // cut for eff charge width &&&
+      delta_length_cut,
+      GoodEdgeAlgo_);


Suggested change

GoodEdgeAlgo_);

goodEdgeAlgo_);

perrotta · 2025-05-06T06:38:52Z

+    shifty = templ.cytemp() - midpix;
+  }  //else
+
+  shifty = BHY - midpix;


This overwrites L479

perrotta · 2025-05-06T06:39:27Z

+  //  midpix = (fypix + lypix) / 2;
+  //  shifty = templ.cytemp() - midpix;


Suggested change

// midpix = (fypix + lypix) / 2;

// shifty = templ.cytemp() - midpix;

mmusich · 2025-05-06T07:30:58Z

Not sure if this is related to the changes.

@mroguljic, do you happen to have run your tests on eos?
This looks related to #44369, thus unrelated.

mroguljic · 2025-05-06T07:35:14Z

Not sure if this is related to the changes.

@mroguljic, do you happen to have run your tests on eos? This looks related to #44369, thus unrelated.

Thank you Marco. Indeed, I ran this on eos

mandrenguyen · 2025-05-07T08:00:48Z

please test

mroguljic · 2025-05-07T08:18:50Z

please test

@mandrenguyen This backport is not yet synchronized to master. If there are no more comments on master, I can do it now.

cmsbuild · 2025-05-09T21:46:07Z

Pull request #48008 was updated. @Martin-Grunewald, @antoniovilela, @atpathak, @cmsbuild, @davidlange6, @fabiocos, @francescobrivio, @fwyzard, @jfernan2, @makortel, @mandrenguyen, @mmusich, @perrotta, @rappoccio can you please check and sign again.

elusian · 2025-05-10T11:07:51Z

Hello, these are the new timing tests with the updated code, with the same conditions as before but with the addition of a test with the PR merged without the customization applied:

Baseline: 481.4 ms
PR without customization: 481.4 ms
PR + customization: 483.0 ms

Of the increase in timing, we have:

+1.5 in doublet recovery track building,
+O(0.1) ms in doublet recovery track fitting
+O(0.1) ms in iter0 track building and fitting
+O(0.1) ms in pixel pair producers
+O(0.1) ms in pixel tracks/cluster conversion
-3.5 ms in other

The small changes are probably within error, I'm reporting them because in principle we can expect some changes there.

mtosi · 2025-05-10T13:55:48Z

thanks @elusian !
so, now we are missing the modifier for the offline and online reconstruction in order to have the physics validation performed, right ?

mroguljic · 2025-05-12T06:24:32Z

thanks @elusian ! so, now we are missing the modifier for the offline and online reconstruction in order to have the physics validation performed, right ?

Hi Mia, we did add modifiers for both offline and online in these files:

Configuration/ProcessModifiers/python/siPixelGoodEdgeAlgo_cff.py
RecoLocalTracker/SiPixelRecHits/python/PixelCPEClusterRepair_cfi.py
RecoLocalTracker/SiPixelRecHits/python/PixelCPEESProducers_cff.py
RecoLocalTracker/SiPixelRecHits/python/PixelCPEGeneric_cfi.py
RecoLocalTracker/SiPixelRecHits/python/PixelCPETemplateReco_cfi.py

fwyzard · 2025-05-12T06:25:58Z

+heterogeneous

mtosi · 2025-05-12T11:30:41Z

thank you, @mroguljic

Configuration/ProcessModifiers/python/siPixelGoodEdgeAlgo_cff.py
RecoLocalTracker/SiPixelRecHits/python/PixelCPEClusterRepair_cfi.py
RecoLocalTracker/SiPixelRecHits/python/PixelCPEESProducers_cff.py
RecoLocalTracker/SiPixelRecHits/python/PixelCPEGeneric_cfi.py
RecoLocalTracker/SiPixelRecHits/python/PixelCPETemplateReco_cfi.py

I'm afraid I don't see the customization for HLT, though
@mmusich @Martin-Grunewald correct me if I'm wrong, but we typically collect such kind of customization function in
HLTrigger/Configuration/python/customizeHLTforCMSSW.py

the customization function should have the following

process.hltESPPixelCPEGeneric.GoodEdgeAlgo = cms.bool(True)
process.hltESPPixelCPEFastParamsPhase1.GoodEdgeAlgo = cms.bool(True)

Martin-Grunewald · 2025-05-12T11:39:16Z

Typically we'd like a CMSHLT Jira ticket to change HLT. The fillDescriptions default should be set as to not use the new feature, ie, preserve the current behaviour (at least at HLT).

mmusich · 2025-05-12T12:02:41Z

I'm afraid I don't see the customization for HLT, though, correct me if I'm wrong, but we typically collect such kind of customization function in HLTrigger/Configuration/python/customizeHLTforCMSSW.py

I'd rather put the customization in a RecolLocalTracker package and then call it directly in the relval machinery.

cmsbuild · 2025-05-13T07:48:55Z

Pull request #48008 was updated. @AdrianoDee, @Martin-Grunewald, @Moanwar, @antoniovilela, @atpathak, @cmsbuild, @davidlange6, @DickyChant, @fabiocos, @francescobrivio, @fwyzard, @jfernan2, @makortel, @mandrenguyen, @miquork, @mmusich, @perrotta, @rappoccio, @srimanob, @subirsarkar can you please check and sign again.

elusian · 2025-05-22T06:48:21Z

Hello, I ran some HLT tracking validation and the plots can be found here.
The tests were done on part of the Winter25 TTbar sample produced with GT 142X_mcRun3_2025_realistic_v7.

To repeat the tests, the steps are (for the reference)

cmsDriver.py step2_Tk25 --conditions 142X_mcRun3_2025_realistic_v7 --datatier GEN-SIM-DIGI-RAW --era Run3_2025 --eventcontent FEVTDEBUGHLT --filein file:TTbar_FEVTDEBUGHLT.root --fileout file:HLT.root --geometry DB:Extended --nStreams 1 --nThreads 1 --no_exec --number -1 --step HLT:Reasonable_2025_menu --process reHLT --customise HLTrigger/Configuration/customizeHLTfor2025Studies.customizeHLTfor2024L1TMenu,HLTrigger/Configuration/customizeHLTfor2025Studies.customizeHLTfor2025Studies

Reasonable_2025_menu was created with

hltGetConfiguration /frozen/2025/2e34/v1.0/HLT/V1 --cff --data --type GRun --path MC_ReducedIterativeTracking_v* > src/HLTrigger/Configuration/python/HLT_Reasonable_2025_menu_cff.py

A new GRun menu should also suffice.
Then the validation is done with

cmsDriver.py val --filein file:HLT.root -s VALIDATION:hltMultiTrackValidation --conditions 142X_mcRun3_2025_realistic_v7 --eventcontent DQM --datatier DQMIO --fileout file:DQMIO.root --nThreads 1 -n -1 --geometry DB:Extended --era Run3_2025 --mc --no_exec --hltProcess reHLT

and the harvesting with

harvestTrackValidationPlots.py DQMIO.root -o DQM.root

Finally, plots are made with

makeTrackValidationPlots.py DQM_ref.root DQM_target1.root DQM_target2.root ... --outputDir www/folder

To test the PR it's enough to modify the conditions following Dinko's comment and use either the procModifier or the customizer.
In the validation step the CPE is not used so in principle the updated conditions are not needed (but just in case this was tested with the black vs orange points in the plots)

mmusich · 2025-06-19T07:01:40Z

-hlt

superseded by the eventual backport of Adapt template pixel cpe algo to better handle shortened or broken clusters #48356

mroguljic · 2025-06-19T09:21:32Z

Closing the PR because it is superseeded by: #48356

cmsbuild added this to the CMSSW_15_0_X milestone May 5, 2025

cmsbuild added reconstruction-pending db-pending operations-pending pending-signatures tests-pending orp-pending trk labels May 5, 2025

mroguljic changed the title ~~change pixel cpe algo to handle broken clusters~~ Adapt pixel cpe algo to better handle broken clusters [15.0.X] May 5, 2025

mroguljic mentioned this pull request May 5, 2025

Adapt pixel cpe algo to better handle broken clusters #47966

Closed

mmusich reviewed May 5, 2025

View reviewed changes

Comment thread CondFormats/SiPixelTransient/src/SiPixelTemplate.cc Outdated

Comment thread CondFormats/SiPixelTransient/src/SiPixelTemplate.cc Outdated

Comment thread RecoLocalTracker/SiPixelRecHits/python/PixelCPEClusterRepair_cfi.py

cmsbuild added the heterogeneous-pending label May 5, 2025

cmsbuild added the backport label May 5, 2025

mroguljic force-pushed the updated_cpe_algo-150X branch from 70746c0 to 680f6cb Compare May 6, 2025 05:55

perrotta reviewed May 6, 2025

View reviewed changes

cmsbuild added tests-started and removed tests-pending labels May 7, 2025

cmsbuild added reconstruction-pending operations-pending tests-pending heterogeneous-pending labels May 9, 2025

cmsbuild added heterogeneous-approved and removed heterogeneous-pending labels May 12, 2025

mroguljic added 4 commits May 13, 2025 09:45

change pixel cpe algo to handle broken clusters

566d073

fix delta in goodEdgeAlgo and don't run it on x projection

d4aba90

fix goodEdgeAlgo CPE alpaka implementation

2afaec8

add goodEdgeAlgo wf and hlt customisation

f1dbf92

mroguljic force-pushed the updated_cpe_algo-150X branch from 133241f to f1dbf92 Compare May 13, 2025 07:48

cmsbuild added pdmv-pending upgrade-pending heterogeneous-pending and removed heterogeneous-approved labels May 13, 2025

cmsbuild added hlt-rejected and removed hlt-pending labels Jun 19, 2025

mroguljic closed this Jun 19, 2025

	GoodEdgeAlgo_ = p.getParameter<bool>("GoodEdgeAlgo");
	goodEdgeAlgo_ = p.getParameter<bool>("GoodEdgeAlgo");

	cout << "GoodEdgeAlgo_ = " << (int)GoodEdgeAlgo_ << endl;
	cout << "goodEdgeAlgo_ = " << (int)GoodEdgeAlgo_ << endl;

		// midpix = (fypix + lypix) / 2;
		// shifty = templ.cytemp() - midpix;

Conversation

mroguljic commented May 5, 2025

Uh oh!

cmsbuild commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmsbuild commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmusich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mmusich commented May 5, 2025

Uh oh!

cmsbuild commented May 5, 2025

Uh oh!

mmusich commented May 5, 2025

Uh oh!

mmusich commented May 5, 2025

Uh oh!

mroguljic commented May 5, 2025

Uh oh!

cmsbuild commented May 6, 2025

Uh oh!

mroguljic commented May 6, 2025

Uh oh!

perrotta 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!

Uh oh!

mmusich commented May 6, 2025

Uh oh!

mroguljic commented May 6, 2025

Uh oh!

mandrenguyen commented May 7, 2025

Uh oh!

mroguljic commented May 7, 2025

Uh oh!

cmsbuild commented May 9, 2025

Uh oh!

elusian commented May 10, 2025

Uh oh!

mtosi commented May 10, 2025

Uh oh!

mroguljic commented May 12, 2025

Uh oh!

fwyzard commented May 12, 2025

Uh oh!

mtosi commented May 12, 2025

Uh oh!

Martin-Grunewald commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmusich commented May 12, 2025

Uh oh!

cmsbuild commented May 13, 2025

Uh oh!

elusian commented May 22, 2025

Uh oh!

cmsbuild commented May 5, 2025 •

edited

Loading

cmsbuild commented May 5, 2025 •

edited

Loading

Martin-Grunewald commented May 12, 2025 •

edited

Loading