D110 and Alpaka (and CUDA) Pixel Tracks Issues
T35/D110 geometry brings some changes w.r.t. D98 that makes unusable the Alpaka (and CUDA) pixel tracks.
- the numbers of modules the layers have changed, making phase2PixelTopology:: layerStart obsolete;
- apparently the pixel pitch is not constant across all modules.
For 1. I see two solutions and I would like to hear other opinions:
- an "easy" one: simply fixing the numbers, that would imply that any different geometry would become unusable (but they should be obsolete now). This could make sense if we know that we won't change them for sure in the future. Unless it becomes a bit annoying to maintain;
- a flexible one: having these numbers actually filled once in an
ESProductreading them from the geometry. This implies more work but has been on my mind since a while also for the strip CA extension. Subject to keeping the same computational performance.
For 2. I don't know if this is really expected but this is something I see also running some standard (CPU) wf using the PixelCPEGeneric (such as 29634.0):
detId : 303087642
x -> this pitch = 0.0025
y -> this pitch = 0.01
detId : 304091160
x -> this pitch = 0.0025
y -> this pitch = 0.0100115
with https://github.com/AdrianoDee/cmssw/commit/231e66a859c9c5ab42226736a0c55551dc12528f on top of 14_1_0_pre3. This could be solved adding the pitch to the GPU DetParams here.
cms-bot internal usage
A new Issue was created by @AdrianoDee.
@rappoccio, @makortel, @sextonkennedy, @Dr15Jones, @antoniovilela, @smuzaffar can you please review it and eventually sign/assign? Thanks.
cms-bot commands are listed here
assign reconstruction
assign trk
New categories assigned: reconstruction
@jfernan2,@mandrenguyen you have been requested to review this Pull request/Issue and eventually sign? Thanks
type trk
type tracking
Note: https://github.com/cms-sw/cmssw/pull/45175#issuecomment-2155832315
assign heterogeneous
I have dysgraphical issues today.
New categories assigned: heterogeneous
@fwyzard,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks
is this pitch "0.0100115" real or just an artifact of some sort of imperfect geometry? (missing some gap somewhere for instance)
Prelude:
T35: Same as T33 with the exception of modified Tracker volume so that it touches CALO on the outer side and BeamPipe on the inner side
T33 was introduced at https://github.com/cms-sw/cmssw/pull/41880. Quoting the description:
This adds a new tracker (T33, workflow D100) with a more realistic description of the 3D modules, which consist of two separate sensors with a gap between them. This is now modelled correctly. The new geometry also updates the sizes of the dead areas in other parts of the inner tracker, to match the latest designs. More details e.g. available here: https://indico.cern.ch/event/1242574/contributions/5382602/attachments/2637975/4564459/em20230428.pdf
@emiglior @adewit FYI
an "easy" one: simply fixing the numbers, that would imply that any different geometry would become unusable (but they should be obsolete now). This could make sense if we know that we won't change them for sure in the future. Unless it becomes a bit annoying to maintain;
The split sensors in BPix1 is a feature meant to stay. OTOH earlier versions without that feature (that have other experimental features), might still be used / relevant, but I let the TRK DPG to comment further on those needs.
So, is the pitch correctly computed or is just moduleWidth/NY w/o accounting for the gap? And is ylocal corectly computed accounting for the right pitch and the gap?
assign upgrade
New categories assigned: upgrade
@srimanob,@subirsarkar you have been requested to review this Pull request/Issue and eventually sign? Thanks
My vote is for the flexible approach:
- a flexible one: having these numbers actually filled once in an
ESProductreading them from the geometry. This implies more work but has been on my mind since a while also for the strip CA extension. Subject to keeping the same computational performance.
For 2. I don't know if this is really expected but this is something I see also running some standard (CPU) wf using the PixelCPEGeneric (such as 29634.0):
Note that in T33/T35, the difference in pitch_Y between TBPX LAY1 modules (3D sensors) and modules in the rest of Inner Tracker (planar sensors) is tiny (0.1 um). Furthermore, in a specific module, the pitch for all the pixels is the same.
Starting from T39, we introduced the so-called BigPixels (PR 44576). Now the size of standard pixels is 25x100 um2, except for pixels lying in the region between two readout chips (CROC) mounted on the same sensors. More in detail:
- "split" 3D sensors (TBPX LAY1) - no BigPixels
- 1x2 planar sensors (TBPX LAY2 TFPX R1-R2) - BigPixels along local-Y (25x225 um2)
- 2x2 planar sensors (TBPX LAY3-LAY4, TFPX R3-R4, TEPX all) - BigPixels along local-X (87.5x100 um2), along local-Y (25x225 um2), and at the crossing (87.5x225 um2)
(see slide#4 of https://indico.cern.ch/event/1415467/contributions/5949441/attachments/2854070/4990838/Phase-2_IT_BigPix_TrkDPG_FollowUp.pdf)
Note that T39 is not yet the baseline Tracker ph-2 geometry, but BigPixels will become a standard feature in the forthcoming ones.
@bardellig @bartosik-hep @Raffaella07
Can somebody point to me WHERE the pitch and the localY is actually computed for the 3 types of sensors?
Can somebody point to me WHERE the pitch and the localY is actually computed for the 3 types of sensors?
In case of Phase II here is the pitch computation: https://github.com/cms-sw/cmssw/blob/master/Geometry/TrackerGeometryBuilder/src/PixelPhase2TopologyBuilder.cc#L27-L28, which was tested for old geometry and geometries with BigPixels information added in the xml
and here localY is computed: https://github.com/cms-sw/cmssw/blob/master/Geometry/TrackerGeometryBuilder/src/RectangularPixelPhase2Topology.cc#L239-L277
done inside the class RectangularPixelPhase2Topology.cc
Ok, how the gap in the "split" 3D sensors (TBPX LAY1) is taken into account into the computation of the pitch?
My vote is for the flexible approach:
Seems the best option also to me. I'll start looking at it.
Ok, I did not realize that the split sensors are two DetId, so no gap to be taken into account... Sorry for the noise.
https://github.com/cms-sw/cmssw/pull/45421 should solve the issue
Closing as #45421 was integrate
@AdrianoDee was https://github.com/cms-sw/cmssw/pull/45421 supposed to fix the issue:
cmsRun: src/RecoLocalTracker/SiPixelRecHits/src/PixelCPEFast.cc:131: void PixelCPEFast<TrackerTraits>::fillParamsForGpu() [with TrackerTraits = pixelTopology::Phase2]: Assertion `commonParamsGPU_.thePitchY == p.thePitchY' failed.
as well? As far as I can see the issue persists in wf 29634.501 and 29634.502.
Thanks!
Yes, for Alpaka but I didn't propagate the fix to CUDA that is still what we run in the relval_gpu.py. I've opened https://github.com/cms-sw/cmssw/pull/45694 to move from CUDA to Alpaka + addition of the triplet workflows. If I can't solve quickly the issue on the triplets that we see there I will just split it in two PRs and have the relval migration in.
I should have fixed it.