-
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
Phase2 tracker: add T30 (geometry with more realistic TFPX) #36660
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36660/27695
|
A new Pull Request was created by @adewit for master. It involves the following packages:
@bbilin, @wajidalikhan, @ianna, @rekovic, @cecilecaillol, @perrotta, @civanch, @yuanchao, @makortel, @cmsbuild, @davidlange6, @Dr15Jones, @epalencia, @cvuosalo, @mdhildreth, @AdrianoDee, @kskovpen, @slava77, @jpata, @qliphy, @fabiocos, @francescobrivio, @malbouis, @jordan-martins, @clacaputo, @srimanob, @tvami can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d44be8/21591/summary.html Comparison SummarySummary:
|
test parameters:
|
@cmsbuild, please test |
It looks like a lot of the new code is a copy (with some modifications) of existing code. In general, this can be a maintenance issue. Has this been already discussed (and agreed) in @cms-sw/tracking-pog-l2 and other groups if this is the desired way forward? |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d44be8/21620/summary.html Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
urgent |
@cms-sw/upgrade-l2 @cms-sw/pdmv-l2 @cms-sw/operations-l2 ping |
+pdmv |
gitHub says no. |
as long as the changes to a file aren't on the same or neighboring lines, git can figure out how to combine them. |
So, it can be signed, no? |
'Geom' : 'Extended2026D91', | ||
'HLTmenu': '@fake2', | ||
'GT' : 'auto:phase2_realistic_T30', | ||
'Era' : 'Phase2C11I13M9', |
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.
The era is not done properly here. D91 uses C17, so the era should be Phase2C17I13M9
. https://github.com/cms-sw/cmssw/blob/master/Configuration/PyReleaseValidation/python/upgradeWorkflowComponents.py#L1583-L1589
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've addressed the comment here: #36931.
BTW @adewit the new D91 geometry is not documented in https://github.com/cms-sw/cmssw/blob/master/Configuration/Geometry/README.md, is it intended?
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.
Thanks @mmusich - the lack of documentation is not intentional. Can we add it in #36931?
In the list of tracker versions:
T30: Phase2 tilted tracker, exploratory geometry *only to be used in D91 for now*. Outer Tracker (v8.0.1): based on v8.0.0 with updated TB2S spacing. Inner Tracker (v6.4.0): based on v6.1.5 but TFPX with more realistic module positions
And at the end of the list of geometries:
D91 = T30+C17+M10+I15+O9+F6
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.
sure. Done in 2ae9367
I've only a comment in Era, but if you would like to merge, then fix in the small PR, I am fine too. |
@srimanob It is probably more efficient to send a second PR to fix this small issue. I'll do it as soon as this PR is merged. |
+Upgrade |
+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. |
PR description:
This adds a new Phase 2 tracker geometry (T30). It is similar to T21/T24, with the main changes occurring in TFPX and TEPX, and a minor change in the OT (updated TB2S module spacing). All sensors in the IT are 25x100 um2 planar
The changes in TFPX and TEPX are:
As a consequence of the last point, a new hierarchy level (subdisk) has been introduced for the forward pixel to address issues observed in the tracking (not all hits from one TP entering the same track, leading to a high prevalence of additional tracks with only a few hits). This new hierarchy level required the addition of a new detID schema, which is used both in TFPX and TEPX, where the same approach is useful.
More information, and validation of these changes, can be found in these slides, where T30 is labelled TX1.
Note: the new hierarchy level, and associated further changes to the code, is only used in T30 for the moment. All other tracker geometries use the already existing phase 2 detID schema and are not affected by the changes implemented here.
@emiglior FYI
PR validation:
if this PR is a backport please specify the original PR and why you need to backport that PR:
Not a backport