Pixel local reco: reduce code duplication between PixelCPEGeneric and PixelCPEFast #33706
Conversation
- common base class for pixel CPE generic classes: PixelCPEGeneric, PixelCPEFast
…asses: - PixelCPEGeneric and PixelCPEFast -> PixelCPEGenericBase
- edgeClusterErrorX_, edgeClusterErrorY_, useErrorsFromTemplates_, truncatePixelCharge_
- reduce duplication in PixelCPEFast and PixelCPEGeneric localError() function
- from PixelCPEGeneric + PixelCPEFast -> PixelCPEGenericBase - use RectangularPixelTopology instead of phase1PixelTopology for edge pixel determination -> in the long term it supports phase 2 integration ? -> this whole commit can be dropped if this is problematic
- cout -> LogDebug
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33706/22619
|
|
A new Pull Request was created by @czangela for master. It involves the following packages: RecoLocalTracker/SiPixelRecHits @perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
enable gpu |
|
@cmsbuild, please test |
|
irradiation corrections could be factored out as they do not depend on cluster details. They can actually applied to hit I am not proposing this: it is just to slow that the CPE can be factorized in two parts: one that computes the position (and the size) and another that computes error and irradiation correction (both may depends on track angle) This can be exploited in patatrack to correct the hit just before the fit see I take the opportunity to ask "stake holder' if they have any plan to move in the direction of this very last PR that is by know pretty obsolete (at least in terms of code) and would require essentially a full re-writing. The last comment from DPG was that indeed improved single hit resolution (at least in terms of pulls). |
| int pixmx; | ||
|
|
||
| // These are errors predicted by PIXELAV | ||
| float sigmay; // CPE Generic y-error for multi-pixel cluster |
There was a problem hiding this comment.
btw: you may have by now realized that there is no need of all these variable as they are mutual exclusive.
cmsbuild
added
code-checks-pending
tests-pending
and removed
code-checks-approved
tests-approved
labels
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33706/22716
|
|
@cmsbuild please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-87155f/15163/summary.html GPU Comparison SummarySummary:
Comparison SummarySummary:
|
|
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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
+1 |
|
On 5/20/21 12:29 AM, Marco Musich wrote:
@cms-sw/trk-dpg-l2 <https://github.com/orgs/cms-sw/teams/trk-dpg-l2>
please confirm that the changes here are OK for you.
This PR has been done in close consultation of the long term maintainers
of this package ***@***.*** <https://github.com/OzAmram> @pmaksim1
<https://github.com/pmaksim1>), therefore we are OK with it.
Thanks
Thanks for clarifying.
It may help for the future PRs, when at creation the PR is already good
for the DPG,
for the reco contact or conveners to leave a note that this is good.
Otherwise, for us/reviewers it's hard to understand.
[we do not need it if a convener or a contact makes a PR]
BTW, can we formally confirm @OzAmram as a pixel reco contact now (we
still have Alice in RecoContacts twiki)?
|
PR description:
This PR aims to reduce some code duplication in the classes
PixelCPEFastandPixelCPEGeneric(underRecoLocalTracker/SiPixelRecHits/).Main changes are:
collect_edge_charges()ClusterParamGenericcommon (PixelCPEFastis planned to have irradiation bias corrections, but I might have misunderstood this)createClusterParam()const std::vector<float> xerr_barrel_l1_, yerr_barrel_l1_, xerr_barrel_ln_, etc.){PixelCPEGeneric|PixelCPEFast}::localError()This should solve the issues listed in #32483 related to "factor common code between
PixelCPEGenericandPixelCPEFast".It introduces a new base class for the two, named
PixelCPEGenericBase, this is where most of the common code went.PR validation:
compilation, runtests, unittests, addOnTests.py --tests=hlt_data_GRun
if this PR is a backport please specify the original PR and why you need to backport that PR:
kindly ping @mmusich @tsusa