Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

D110 and Alpaka (and CUDA) Pixel Tracks Issues #45177

Closed
AdrianoDee opened this issue Jun 8, 2024 · 28 comments
Closed

D110 and Alpaka (and CUDA) Pixel Tracks Issues #45177

AdrianoDee opened this issue Jun 8, 2024 · 28 comments

Comments

@AdrianoDee
Copy link
Contributor

AdrianoDee commented Jun 8, 2024

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 AdrianoDee@231e66a on top of 14_1_0_pre3. This could be solved adding the pitch to the GPU DetParams here.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2024

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

@AdrianoDee
Copy link
Contributor Author

AdrianoDee commented Jun 8, 2024

assign reconstruction

@AdrianoDee
Copy link
Contributor Author

assign trk

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2024

New categories assigned: reconstruction

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

@AdrianoDee
Copy link
Contributor Author

type trk

@AdrianoDee
Copy link
Contributor Author

type tracking

@AdrianoDee
Copy link
Contributor Author

Note: #45175 (comment)

@AdrianoDee
Copy link
Contributor Author

assign heterogeneous

I have dysgraphical issues today.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2024

New categories assigned: heterogeneous

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

@VinInn
Copy link
Contributor

VinInn commented Jun 8, 2024

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

@mmusich
Copy link
Contributor

mmusich commented Jun 8, 2024

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 #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
Copy link
Contributor

mmusich commented Jun 8, 2024

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.

@VinInn
Copy link
Contributor

VinInn commented Jun 8, 2024

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?

@srimanob
Copy link
Contributor

srimanob commented Jun 8, 2024

assign upgrade

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2024

New categories assigned: upgrade

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

@fwyzard
Copy link
Contributor

fwyzard commented Jun 10, 2024

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.

@emiglior
Copy link
Contributor

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

@VinInn
Copy link
Contributor

VinInn commented Jun 10, 2024

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

@bardellig
Copy link
Contributor

bardellig commented Jun 10, 2024

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

@VinInn
Copy link
Contributor

VinInn commented Jun 10, 2024

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

@AdrianoDee
Copy link
Contributor Author

My vote is for the flexible approach:

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

@VinInn
Copy link
Contributor

VinInn commented Jun 11, 2024

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

@AdrianoDee
Copy link
Contributor Author

AdrianoDee commented Jul 10, 2024

#45421 should solve the issue

@AdrianoDee
Copy link
Contributor Author

Closing as #45421 was integrate

@mmusich
Copy link
Contributor

mmusich commented Aug 20, 2024

@AdrianoDee was #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!

@AdrianoDee
Copy link
Contributor Author

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 #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
Copy link
Contributor Author

I should have fixed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants