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

ETL Geometry V8 #43407

Merged
merged 3 commits into from
Dec 12, 2023
Merged

ETL Geometry V8 #43407

merged 3 commits into from
Dec 12, 2023

Conversation

24LopezR
Copy link
Contributor

@24LopezR 24LopezR commented Nov 27, 2023

PR description:

This PR implements new version of ETL geometry (v8) that includes modules with several LGAD sensors (2 in this case). This geometry is integrated in MTD I17 and CMS D105.
The details about the motivation and implementation can be found in these presentations:

PR validation:

For validation, a TTbar 14TeV has been run with the nominal code in D98 geometry, and the new code in D98 and D105 geometries. We observe (see attached plot (plot legend says D104 but ultimately D104->D105)):

  1. Older geometries remain untouched.
  2. The size of the ETL sensor is halved as expected.
  3. The number of events recorded is about 10% higher, as the logical area of the pixels has been increased.

image

Added runTheMatrix test workflow 27634.0 for this scenario 2026D105.

@parbol @fabiocos

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @24LopezR (Ruben Lopez) for master.

It involves the following packages:

  • Configuration/Geometry (geometry, upgrade)
  • Configuration/PyReleaseValidation (pdmv, upgrade)
  • Configuration/StandardSequences (operations)
  • DataFormats/ForwardDetId (simulation, upgrade)
  • Geometry/CMSCommonData (geometry, upgrade)
  • Geometry/MTDCommonData (geometry, upgrade)
  • Geometry/MTDGeometryBuilder (geometry, upgrade)
  • Geometry/MTDNumberingBuilder (geometry, upgrade)
  • Geometry/MTDSimData (geometry, upgrade)
  • RecoMTD/DetLayers (upgrade, reconstruction)
  • SimFastTiming/FastTimingCommon (simulation, upgrade)

@miquork, @mandrenguyen, @cmsbuild, @AdrianoDee, @srimanob, @Dr15Jones, @fabiocos, @jfernan2, @bsunanda, @davidlange6, @sunilUIET, @antoniovilela, @rappoccio, @mdhildreth, @makortel, @civanch can you please review it and eventually sign? Thanks.
@vargasa, @GiacomoSguazzoni, @youyingli, @slomeo, @dgulhan, @rovere, @apsallid, @Martin-Grunewald, @fabiocos, @felicepantaleo, @missirol, @mtosi, @bsunanda, @sameasy, @VourMa, @VinInn, @makortel, @JanFSchulte, @mmusich this is something you requested to watch as well.
@rappoccio, @antoniovilela, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@fabiocos
Copy link
Contributor

please test

@fabiocos
Copy link
Contributor

@srimanob @bsunanda this test will not probe the new geometry, just confirm no impact on the older one. In order to explicitly test the new geometry we should add the new (D104) scenario to the short matrix. Do you agree on this addition?

@srimanob
Copy link
Contributor

Hi @fabiocos
It depends on how we will move next. Currently, we are at D98 and moving to D99. If we woudl like to jump to D101 first, or we jump to MTD first. However, both option include C18.

D99 = T32+C18+M10+I16+O9+F8
D100 = T34+C17+M11+I16+O9+F8
D101 = T34+C18+M11+I16+O9+F8
D102 = T35+C17+M11+I16+O9+F8
D103 = T34+C21+M11+I16+O9+F8
D104 = T34+C21+M11+I17+O9+F8

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e10051/36114/summary.html
COMMIT: e82c49d
CMSSW: CMSSW_14_0_X_2023-11-27-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/43407/36114/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found 2 errors in the following unit tests:

---> test GeometryMTDNumberingBuilderTestDriver had ERRORS
---> test test2026Geometry had ERRORS

Comparison Summary

Summary:

@fabiocos
Copy link
Contributor

@24LopezR the first failed unit test is a consequence of the change in the top level ETL DetIds I was telling you a few days ago. If we want to keep this we need to update accordingly the reference files.

@fabiocos
Copy link
Contributor

@24LopezR the D104 geometry configuration files need to be ri-generated by rerunning the script

python3 Configuration/Geometry/scripts/generate2026Geometry.py -D104

Changes are unrelated to MTD, I assume updates recently introduced

@fabiocos
Copy link
Contributor

@24LopezR looking at the D104 definition here, I wonder whether this si what we want for tracker and calorimeters. @srimanob @bsunanda probably we should base this on top of D99 that is the new baseline, right? The only important thing is that tracker and calo envelope are compatible with BTL and ETL respectively, and in both T32 and T34 they are, Calo envelope around ETL isn't changed to my knowledge.

@fabiocos
Copy link
Contributor

@24LopezR please update D104 basing it on D99 as starting point, i.e. just D104 = T32+C18+M10+I17+O9+F8

@bsunanda
Copy link
Contributor

bsunanda commented Dec 8, 2023

+geometry

@srimanob
Copy link
Contributor

srimanob commented Dec 8, 2023

@24LopezR could you please fix PR description to D105 to avoid future confusion. Thx.

@24LopezR
Copy link
Contributor Author

24LopezR commented Dec 8, 2023

@srimanob Done! :)

@srimanob
Copy link
Contributor

srimanob commented Dec 8, 2023

+Upgrade

The new workflow of D105 runs fine.

@civanch
Copy link
Contributor

civanch commented Dec 8, 2023

+1

@bsunanda
Copy link
Contributor

bsunanda commented Dec 8, 2023

@sunilUIET Please approve this PR

@sunilUIET
Copy link
Contributor

+pdmv

@mandrenguyen
Copy link
Contributor

+reconstruction
Nothing changes to comparisons, but it seems from this comment #43407 (comment)
that the output is sensible

@fabiocos
Copy link
Contributor

@cms-sw/orp-l2 the additions to the operations area are the usual set of workflows for scenario D105, and the availability of one of them in the short matrix. Do you see issues with that? Otherwise can we move forward with the integration of this PR? I have ready its extension to BTL exploiting the same geometry scenario.

@antoniovilela
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

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.

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

Successfully merging this pull request may close these issues.

9 participants