cmssw icon indicating copy to clipboard operation
cmssw copied to clipboard

D110 and Alpaka (and CUDA) Pixel Tracks Issues

Open AdrianoDee opened this issue 1 year ago • 23 comments

T35/D110 geometry brings some changes w.r.t. D98 that makes unusable the Alpaka (and CUDA) pixel tracks.

  1. the numbers of modules the layers have changed, making phase2PixelTopology:: layerStart obsolete;
  2. 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 ESProduct reading 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.

AdrianoDee avatar Jun 08 '24 10:06 AdrianoDee

cms-bot internal usage

cmsbuild avatar Jun 08 '24 10:06 cmsbuild

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

cmsbuild avatar Jun 08 '24 10:06 cmsbuild

assign reconstruction

AdrianoDee avatar Jun 08 '24 10:06 AdrianoDee

assign trk

AdrianoDee avatar Jun 08 '24 10:06 AdrianoDee

New categories assigned: reconstruction

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

cmsbuild avatar Jun 08 '24 10:06 cmsbuild

type trk

AdrianoDee avatar Jun 08 '24 10:06 AdrianoDee

type tracking

AdrianoDee avatar Jun 08 '24 10:06 AdrianoDee

Note: https://github.com/cms-sw/cmssw/pull/45175#issuecomment-2155832315

AdrianoDee avatar Jun 08 '24 11:06 AdrianoDee

assign heterogeneous

I have dysgraphical issues today.

AdrianoDee avatar Jun 08 '24 11:06 AdrianoDee

New categories assigned: heterogeneous

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

cmsbuild avatar Jun 08 '24 11:06 cmsbuild

is this pitch "0.0100115" real or just an artifact of some sort of imperfect geometry? (missing some gap somewhere for instance)

VinInn avatar Jun 08 '24 13:06 VinInn

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

mmusich avatar Jun 08 '24 13:06 mmusich

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.

mmusich avatar Jun 08 '24 14:06 mmusich

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?

VinInn avatar Jun 08 '24 14:06 VinInn

assign upgrade

srimanob avatar Jun 08 '24 15:06 srimanob

New categories assigned: upgrade

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

cmsbuild avatar Jun 08 '24 15:06 cmsbuild

My vote is for the flexible approach:

  • a flexible one: having these numbers actually filled once in an ESProduct reading 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.

fwyzard avatar Jun 10 '24 12:06 fwyzard

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

emiglior avatar Jun 10 '24 13:06 emiglior

Can somebody point to me WHERE the pitch and the localY is actually computed for the 3 types of sensors?

VinInn avatar Jun 10 '24 13:06 VinInn

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

bardellig avatar Jun 10 '24 13:06 bardellig

Ok, how the gap in the "split" 3D sensors (TBPX LAY1) is taken into account into the computation of the pitch?

VinInn avatar Jun 10 '24 14:06 VinInn

My vote is for the flexible approach:

Seems the best option also to me. I'll start looking at it.

AdrianoDee avatar Jun 10 '24 18:06 AdrianoDee

Ok, I did not realize that the split sensors are two DetId, so no gap to be taken into account... Sorry for the noise.

VinInn avatar Jun 11 '24 06:06 VinInn

https://github.com/cms-sw/cmssw/pull/45421 should solve the issue

AdrianoDee avatar Jul 10 '24 13:07 AdrianoDee

Closing as #45421 was integrate

AdrianoDee avatar Aug 13 '24 14:08 AdrianoDee

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

mmusich avatar Aug 20 '24 07:08 mmusich

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.

AdrianoDee avatar Aug 20 '24 07:08 AdrianoDee

I should have fixed it.

AdrianoDee avatar Aug 20 '24 11:08 AdrianoDee