Duplication of clusterizers for bricked pixels

[jpata]

The following files were duplicated with minor modifications in #35441.

• RecoLocalTracker/SiPixelClusterizer
  • plugins/PixelThresholdClusterizer.h -> plugins/PixelThresholdClusterizerForBricked.h
  • plugins/PixelThresholdClusterizer.cc -> pluginsPixelThresholdClusterizerForBricked.cc
• RecoLocalTracker/SiPixelRecHits
  • interface/PixelCPEGeneric.h -> interface/PixelCPEGenericForBricked.h
  • src/PixelCPEGeneric.cc -> src/PixelCPEGenericForBricked.cc

Per the comments #35441 (comment), the duplication should be reviewed as soon as the studies with bricked pixels are done, and subsequently removed from the codebase (or otherwise rewritten to avoid duplication).

[jpata]

assign reconstruction

[cmsbuild]

New categories assigned: reconstruction

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

[cmsbuild]

A new Issue was created by @jpata Joosep Pata.

@Dr15Jones, @perrotta, @dpiparo, @makortel, @smuzaffar, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[jpata]

@franzglessgen can you let us know if there's any update on this? Are the studies concluded?

[franzglessgen]

Hi ! I do not think the studies are over yet, @emiglior might be more up to date about the latest development.

[emiglior]

Studies on Tracker geometries featuring bricked pixels are not done yet (ETA: end of January 2022)

[jpata]

a kind ping on this issue.

[jpata]

@franzglessgen @emiglior a kind ping on this issue

[emiglior]

Hi Josep, indeed the bricked pixel option is currently disfavored. We will have the final decision (and therefore the clean up of the code) in 1 month.

@franzglessgen

[jpata]

type trk

[jpata]

@emiglior @franzglessgen kind ping on this.

[jpata]

@emiglior @franzglessgen any news?

[mmusich]

The last bricked pixel geometry in cmssw is removed at #38061.
I guess that after that is merged support for a special local reco could be dropped afterwards, thus addressing this issue.

[emiglior]

Indeed the plan is to drop the special local reco for bricked pixels

The question is if removing the classes below is enough
RecoLocalTracker/SiPixelClusterizer
plugins/PixelThresholdClusterizerForBricked.h
pluginsPixelThresholdClusterizerForBricked.cc
RecoLocalTracker/SiPixelRecHits
interface/PixelCPEGenericForBricked.h
src/PixelCPEGenericForBricked.cc

or if you want to revert changes in the base classes as well

Few examples:

• remove the generic_position_formula_y_bricked in SiPixelUtils.h
• revert from protected to private in PixelThresholdClusterizer.h

@franzglessgen

[mmusich]

@jpata @cms-sw/reconstruction-l2

can you give your opinion on #35519 (comment)

[jpata]

The changes introduced in #35441 that only support the bricked pixels and won't be tested/used/supported in the future should be removed.

Changes that were introduced in that PR that generally support code reuse in pixel local reco could stay.

[franzglessgen]

@emiglior I can remove those classes. Did I understand correctly that this should wait until this PR is merged ? Let me know when I can go ahead with the removal.
@jpata so the additional functions that were implemented within already existing classes can stay right ?

[jpata]

so the additional functions that were implemented within already existing classes can stay right ?

I would say so, but I don't have an overview in front of me now, so you can exercise some judgement. My main point was that code that becomes unused and thus untested should be removed. Thanks for taking care of this!

[emiglior]

@franzglessgen yes, you have to wait that PR#38061 is merged

[mmusich]

now that #38061 is merged, can we remove the duplicated code?

[franzglessgen]

Thanks for the notification, I am going on holiday this week so I'll start to clean up the code mid July if that's okay.

[tvami]

@franzglessgen please note #38529

[franzglessgen]

Oh it has been done already, thanks for the info!

[jpata]

+reconstruction

• solved in Remove Phase-2 bricked pixels specific reconstruction code from CMSSW #38529

[cmsbuild]

This issue is fully signed and ready to be closed.

[franzglessgen]

Hi! Thanks for the ping. I am not aware of the latest status but if I get the green light by Ernesto I could remove this piece of code. Cheers, Franz Le jeu. 12 mai 2022 à 16:01, Joosep Pata ***@***.***> a écrit :

…

@emiglior <https://github.com/emiglior> @franzglessgen <https://github.com/franzglessgen> any news? — Reply to this email directly, view it on GitHub <#35519 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANU4VI6I3GERMYROUDTKWK3VJUFNNANCNFSM5FIUOBXQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>