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.

Sign up for GitHub

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

Jump to bottom

MTD RECO geometry: MTDNumberingBuilder code simplification #28997

Merged
merged 9 commits into from
Feb 27, 2020
Merged

MTD RECO geometry: MTDNumberingBuilder code simplification #28997

merged 9 commits into from
Feb 27, 2020

Conversation

fabiocos
Copy link
Contributor

PR description:

The existing MTD RECO geometry is based on a clone and minimal adjustment of the existing tracker geometry performed mostly by @lgray in view of the MTD TDR release. The need to 1) add the new ETL design and 2) to migrate this code to DD4hep strengthen the need for a code simplification and refresh, already emerged in the 2018 review and in more recent discussions with @ianna.

This PR is the first step in this direction: the aim is to preserve the existing geometry description, to be tested in detail through the refreshed test classes in Geometry/MTDNumberingBuilder and Geometry/MTDGeometryBuilder, while moving from a layered build of the GeometricTimingDet structure from DDD to a much simpler overall loop, where only the subdetectors (BTL and ETL, two sides) and the final sensitive modules are kept (and appear necessary).
Several intermediate classes are removed, with no apparent impact on the final geometry structure produced in MTDGeometryBuilder.

The following step is the modification of the DDDCmsMTDConstruction class (and auxiliary code) to include the new geometry structure of ETL as implemented by @icosivi at xml level.

PR validation:

The dumps of the geometries, provided the same version of the test analyzers in mtd_cfg.py for the benchmark scenario D50, differ only in GeometricTimingDetAnalyzer for the fix of the type of modules performed in GeometricTimingDet (but apparently not relevant in the final construction of the geometry):

173c100
< Module = BModule33Layer1NegativeZTimingwafer type = 4 rawId = 1652669440 Sub/side/RR = 1 0 1
---
> Module = BModule33Layer1NegativeZTimingwafer type = 100 rawId = 1652669440 Sub/side/RR = 1 0 1

79638c72655
< Module = EModule1Disc1Timingwafer type = 10 rawId = 1665204352 Sub/side/RR = 2 1 1
---
> Module = EModule1Disc1Timingwafer type = 100 rawId = 1665204352 Sub/side/RR = 2 1 1

@cmsbuild cmsbuild added this to the CMSSW_11_1_X milestone Feb 19, 2020
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28997/13838

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @fabiocos (Fabio Cossutti) for master.

It involves the following packages:

Geometry/MTDNumberingBuilder

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88 can you please review it and eventually sign? Thanks.
@davidlange6, @silviodonato, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@fabiocos
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 19, 2020 •
edited

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/4777/console Started: 2020/02/19 15:40

@cmsbuild
Copy link
Contributor

+1
Tested at: 47d955c
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-56cf13/4777/summary.html
CMSSW: CMSSW_11_1_X_2020-02-19-1100
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-56cf13/4777/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2694086
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2693766
  • DQMHistoTests: Total skipped: 319
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@cvuosalo
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor Author

@kpedro88 comments?

@fabiocos
Copy link
Contributor Author

@cvuosalo @kpedro88 @silviodonato for the time being, unless you have further comments, I consider this PR as stable, and defer further developments to the next PR I am preparing, essentially adapt this new structure to include the new ETL design (which has a different structure and requires some non-trivial code adjustment).

@kpedro88
Copy link
Contributor

+upgrade

@cvuosalo
Copy link
Contributor

+1

@silviodonato
Copy link
Contributor

please test workflow 23234.1001, 23434.1001

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 26, 2020 •
edited

The tests are being triggered in jenkins.
Test Parameters:

@cmsbuild
Copy link
Contributor

+1
Tested at: ab55b77
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-56cf13/4884/summary.html
CMSSW: CMSSW_11_1_X_2020-02-25-2300
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-56cf13/4884/summary.html

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-56cf13/11634.0_TTbar_14TeV+TTbar_14TeV_TuneCP5_2021_GenSimFull+DigiFull_2021+RecoFull_2021+HARVESTFull_2021+ALCAFull_2021
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-56cf13/12434.0_TTbar_14TeV+TTbar_14TeV_TuneCP5_2023_GenSimFull+DigiFull_2023+RecoFull_2023+HARVESTFull_2023+ALCAFull_2023
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-56cf13/20034.0_TTbar_14TeV+TTbar_14TeV_TuneCP5_2026D35_GenSimHLBeamSpotFull14+DigiFullTrigger_2026D35+RecoFullGlobal_2026D35+HARVESTFullGlobal_2026D35
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-56cf13/20434.0_TTbar_14TeV+TTbar_14TeV_TuneCP5_2026D41_GenSimHLBeamSpotFull14+DigiFullTrigger_2026D41+RecoFullGlobal_2026D41+HARVESTFullGlobal_2026D41
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-56cf13/21234.0_TTbar_14TeV+TTbar_14TeV_TuneCP5_2026D44_GenSimHLBeamSpotFull14+DigiFullTrigger_2026D44+RecoFullGlobal_2026D44+HARVESTFullGlobal_2026D44
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-56cf13/23234.0_TTbar_14TeV+TTbar_14TeV_TuneCP5_2026D49_GenSimHLBeamSpotFull14+DigiFullTrigger_2026D49+RecoFullGlobal_2026D49+HARVESTFullGlobal_2026D49
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-56cf13/23234.1001_TTbar_14TeV+RecoFullGlobal_TestOldDigi_2026D49+HARVESTFullGlobal_2026D49
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-56cf13/23434.1001_TTbar_14TeV+RecoFullGlobalPU_TestOldDigi_2026D49PU+HARVESTFullGlobalPU_2026D49PU

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2679706
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2679386
  • DQMHistoTests: Total skipped: 319
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@silviodonato
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 71d054b into cms-sw:master Feb 27, 2020
@fabiocos fabiocos deleted the fc-mtd-newrecogeom branch March 2, 2020 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kpedro88 kpedro88 kpedro88 left review comments

Assignees
No one assigned
Labels
code-checks-approved comparison-available fully-signed geometry-approved orp-approved tests-approved upgrade-approved
Projects
None yet
Milestone
CMSSW_11_1_X
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@fabiocos @cmsbuild @cvuosalo @kpedro88 @silviodonato