-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Phase 2 tracker: realistic description of 3D modules in inner tracker #41880
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41880/35800
|
A new Pull Request was created by @adewit for master. It involves the following packages:
@perrotta, @rappoccio, @Dr15Jones, @bsunanda, @makortel, @bbilin, @civanch, @mdhildreth, @tvami, @cmsbuild, @AdrianoDee, @srimanob, @saumyaphor4252, @clacaputo, @kskovpen, @sunilUIET, @francescobrivio, @mandrenguyen, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
from Geometry.TrackerGeometryBuilder.trackerParameters_cff import * | ||
from Geometry.TrackerNumberingBuilder.trackerTopology_cfi import * | ||
from Geometry.TrackerGeometryBuilder.idealForDigiTrackerGeometry_cff import * | ||
trackerGeometry.applyAlignment = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? #41075
from Geometry.TrackerGeometryBuilder.trackerParameters_cff import * | ||
from Geometry.TrackerNumberingBuilder.trackerTopology_cfi import * | ||
from Geometry.TrackerGeometryBuilder.idealForDigiTrackerGeometry_cff import * | ||
trackerGeometry.applyAlignment = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? #41075
'from Geometry.TrackerGeometryBuilder.trackerParameters_cff import *', | ||
'from Geometry.TrackerNumberingBuilder.trackerTopology_cfi import *', | ||
'from Geometry.TrackerGeometryBuilder.idealForDigiTrackerGeometry_cff import *', | ||
'trackerGeometry.applyAlignment = False', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? #41075
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the omission and thanks for spotting it - T33 was cloned from T25 before 41075, and I missed the change afterwards. Now corrected.
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41880/35801
|
Pull request #41880 was updated. @perrotta, @rappoccio, @Dr15Jones, @bsunanda, @makortel, @bbilin, @civanch, @mdhildreth, @tvami, @cmsbuild, @AdrianoDee, @srimanob, @saumyaphor4252, @clacaputo, @kskovpen, @sunilUIET, @francescobrivio, @mandrenguyen, @fabiocos, @davidlange6 can you please check and sign again. |
test parameters:
|
Hi @adewit |
@cmsbuild please test |
As we do not observe large differences in physics performance between T33/D100 and T32/D98 (and considering that T33 is not yet the final TBPX layout), it is not strictly necessary adopting T33 as new baseline. A possible exception is if 13_2_X will be used for the production of large MC samples of interest for the Tracker alignment. The reason is that T33 is introducing a new level in the hierarchy and a new detid scheme for TBPX layer1, and perhaps there is in some interest in the TkAl group to start to play with this geometry. |
+pdmv |
@bsunanda @adewit please notice that it is now too late for entering a 17k line code into pre2, which we plan to build within minutes. This PR will be taken into account for the next pre-release |
That is fine. I want an IB with this PR included. I need to modify the dictionary for both Run3 and Phase2 scenarios for some corrections. One of them is rather urgent (RPC geometry) |
+1 |
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 be automatically merged. |
unsigned int layerMask_; | ||
unsigned int ladderMask_; | ||
unsigned int moduleMask_; | ||
unsigned int doubleMask_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adewit , we have couple of warning in IBs due to this change. Can you please provide a fix with correct default values for these newly added member here https://github.com/cms-sw/cmssw/blob/master/DataFormats/TrackReco/test/testHitPattern.cpp#L16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @smuzaffar - since this member is not used for phase 0 or 1 I will just 0-initialize it in testHitPattern.cpp. Given the unit tests apparently didn't reveal this problem, what test(s) should I run locally to make sure everything is now OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, just recompiling DataFormats/TrackReco
will tell.
- In the case of the phase-2 IT there is an additional layer of hierarcy, due ot split sensors in Layer 1. First introduced in PR cms-sw#41880
- In the case of the phase-2 IT there is an additional layer of hierarcy, due ot split sensors in Layer 1. First introduced in PR cms-sw#41880
- In the case of the phase-2 IT there is an additional layer of hierarcy, due ot split sensors in Layer 1. First introduced in PR cms-sw#41880
- In the case of the phase-2 IT there is an additional layer of hierarcy, due ot split sensors in Layer 1. First introduced in PR cms-sw#41880
- In the case of the phase-2 IT there is an additional layer of hierarcy, due ot split sensors in Layer 1. First introduced in PR cms-sw#41880
- In the case of the phase-2 IT there is an additional layer of hierarcy, due ot split sensors in Layer 1. First introduced in PR cms-sw#41880
@@ -27,6 +28,7 @@ | |||
'T21' : ( ','.join( [ 'SiPixelLorentzAngle_phase2_T15_v5_mc' ,SiPixelLARecord,connectionString, "", "2020-05-05 20:00:00.000"] ), ), #uH = 0.053/T (TBPX), uH=0.0/T (TEPX+TFPX) | |||
'T25' : ( ','.join( [ 'SiPixelLorentzAngle_phase2_T25_v0_mc' ,SiPixelLARecord,connectionString, "", "2021-03-16 20:00:00.000"] ), ), #uH = 0.053/T (TBPX L2,L3,L4), uH=0.0/T (TBPX L1 TEPX+TFPX) | |||
'T30' : ( ','.join( [ 'SiPixelLorentzAngle_phase2_IT_v6.4.0_25x100_v1_mc' ,SiPixelLARecord,connectionString, "", "2021-11-22 21:00:00.000"] ), ), #uH = 0.053/T (TBPX), uH=0.0/T (TEPX+TFPX) | |||
'T33' : ( ','.join( [ 'SiPixelLorentzAngle_phase2_IT_v7.1.1_25x100_v1_mc' ,SiPixelLARecord,connectionString, "", "2023-05-16 20:00:00.000"] ), ), #uH = 0.053/T (TBPX L2,L3,L4), uH=0.0/T (TBPX L1 TEPX+TFPX) | |||
} | |||
|
|||
allTags["LAWidth"] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the record, this section of the map should have been updated as well, see #43466 (comment) and following.
- In the case of the phase-2 IT there is an additional layer of hierarcy, due ot split sensors in Layer 1. First introduced in PR cms-sw#41880
PR 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 FYI
PR validation:
Code checks+format done; verified that standard workflows still run.
Track validation inspected (http://personalpages.to.infn.it/~migliore/T25std_T25split/FourMu_NoSmear/plots-new/index.html) and looks as expected.
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