Remove Phase-2 IT bricked design

[tvami]

PR description:

• Removed PixelBrickedDigitizerAlgorithm since that design was discarded
• Removed generic_position_formula_y_bricked which seems to have been a remnant from the cleanup from the local reco
• since I was there I changed the namespace naming to obey rule 2.7
• Removed the bricked design from the topology in 11e07be

PR validation:

• Checked the dependencies if there is anything that wouldnt compile
• Ran wf 20834.0 to make sure the default Ph2 digitizer works

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:

Not a backport, and no backport needed

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40443/33592

• This PR adds an extra 24KB to repository

• There are other open Pull requests which might conflict with changes you have proposed:

  • File SimTracker/SiPhase2Digitizer/plugins/PixelBrickedDigitizerAlgorithm.cc modified in PR(s): Include charge reweighting for Phase-2 IT planar sensors #40417
  • File SimTracker/SiPhase2Digitizer/plugins/PixelBrickedDigitizerAlgorithm.h modified in PR(s): Include charge reweighting for Phase-2 IT planar sensors #40417
  • File SimTracker/SiPhase2Digitizer/python/phase2TrackerDigitizer_cfi.py modified in PR(s): Include charge reweighting for Phase-2 IT planar sensors #40417

[cmsbuild]

A new Pull Request was created by @tvami (Tamas Vami) for master.

It involves the following packages:

• CondFormats/SiPixelTransient (db, reconstruction)
• RecoLocalTracker/SiPixelRecHits (reconstruction)
• SimTracker/SiPhase2Digitizer (upgrade, simulation)

@malbouis, @civanch, @clacaputo, @cmsbuild, @AdrianoDee, @srimanob, @saumyaphor4252, @mdhildreth, @ggovi, @tvami, @mandrenguyen, @francescobrivio can you please review it and eventually sign? Thanks.
@mtosi, @beaucero, @VourMa, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @mroguljic, @missirol, @dkotlins, @ferencek, @trtomei, @gpetruc, @mmusich, @threus, @dgulhan, @seemasharmafnal, @tvami this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

[tvami]

@cmsbuild , please test

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d435b4/29821/summary.html
COMMIT: 3befcf2
CMSSW: CMSSW_13_0_X_2023-01-06-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40443/29821/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• No significant changes to the logs found
• Reco comparison results: 33 differences found in the comparisons
• DQMHistoTests: Total files compared: 49
• DQMHistoTests: Total histograms compared: 3555748
• DQMHistoTests: Total failures: 1220
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3554506
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
• Checked 211 log files, 162 edm output root files, 49 DQM output files
• TriggerResults: no differences found

[mmusich]

hold

• has this been discussed in the phase-2 simulation meeting ? @emiglior . Is there any residual interest in this for phase-3 upgrades?

[cmsbuild]

Pull request has been put on hold by @mmusich
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

[tvami]

has this been discussed in the phase-2 simulation meeting ? @emiglior

It was discussed with Ernesto, I added you to the email thread now

[mmusich]

It was discussed with Ernesto, I added you to the email thread now

thanks. Supporting storage for the bricked pixel information entailed much more involved changes in the base tracker geometry classes layouts than what is removed here (see b462e3b). I am wondering if one should try to address that first is there is really no usage for bricked-ness simulation at any level.

[emiglior]

It was discussed with Ernesto, I added you to the email thread now

thanks. Supporting storage for the bricked pixel information entailed much more involved changes in the base tracker geometry classes layouts than what is removed here (see b462e3b). I am wondering if one should try to address that first is there is really no usage for bricked-ness simulation at any level.

Perhaps I am misunderstanding your point: Do you mean remove all the occurrences of "isBricked()" (and similar) or also the include of the header files (e.g. PTrackerAdditionalParametersPerDetRcd.h) through which the brickedness was implemented?

Meanwhile, I will cross check again with people working on ph-3 about their actual plans on bricked pixels.

[mmusich]

Perhaps I am misunderstanding your point: Do you mean remove all the occurrences of "isBricked()" (and similar) or also the include of the header files (e.g. PTrackerAdditionalParametersPerDetRcd.h) through which the brickedness was implemented?

yes, carrying around this additional complication for no apparent reason doesn't not seem very sustainable for maintenance reasons in the long run.

[tvami]

I can do another commit which will remove bricked from the topology, but only later today

[tvami]

So there is an isBricked() in the MTD
https://github.com/cms-sw/cmssw/blob/master/Geometry/MTDGeometryBuilder/interface/RectangularMTDTopology.h#L173
not really sure what to do with this, or if it has anything to do with the IT topology.

[mmusich]

not really sure what to do with this, or if it has anything to do with the IT topology.

That's necessary because RectangularMTDTopology inherits from PixelTopology (which provides the interface).
Once it's removed in the base class it can naturally be removed here as well. As you can see anyway always returns false.

[smuzaffar]

@mmusich , RecoTracker at https://github.com/cms-sw/cms-bot/blob/master/categories_map.py#L1786 matched file Configuration/Geometry/python/GeometryRecoTracker_cff.py that is why bot added the tracking label :-)

[clacaputo]

+reconstruction

[mmusich]

RecoTracker at https://github.com/cms-sw/cms-bot/blob/master/categories_map.py#L1786 matched file Configuration/Geometry/python/GeometryRecoTracker_cff.py that is why bot added the tracking label

@smuzaffar hmm, would RecoTracker/ avoid that?
That's a clearly unintended match :-)

[smuzaffar]

would RecoTracker/ avoid that?

yes

[AdrianoDee]

+upgrade

[AdrianoDee]

I was reading the thread: is it there at the end something that documents this decision?

[mmusich]

@emiglior

Meanwhile, I will cross check again with people working on ph-3 about their actual plans on bricked pixels.

Did phase-3 people get back to you about it?

[cmsbuild]

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. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

[mmusich]

is it there at the end something that documents this decision?

I guess it will be discussed today at the ph2 tracker simulation meeting

[AdrianoDee]

I see , thanks!

[emiglior]

@emiglior

Meanwhile, I will cross check again with people working on ph-3 about their actual plans on bricked pixels.

Did phase-3 people get back to you about it?

please go on with the removal

[tvami]

hi @cms-sw/orp-l2 can we merge this soon, before I'll need to rebase again? Thanks!

[perrotta]

+1

• Phase2 tracker approved it, see Remove Phase-2 IT bricked design #40443 (comment)
• The removals applied in quite several files did not break the builds, nor resulted in any visible difference in the test outputs

[perrotta]

@tvami a unit test failure in the IBs shows that there is a remaining inclusion of TrackerAdditionalParametersPerDet_cfi.py in DQM/SiStripCommon/test/testTkHistoMap_cfg.py: could you please have a look?

[tvami]

ok, I'm on it