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

MTD Geometry - BTL numbering scheme update with correction for backward compatibility #47353

Merged
merged 1 commit into from
Mar 7, 2025

Conversation

cquarant
Copy link
Contributor

PR description:

This PR is the re-implementation of PR #46977, that was reverted by the following PR #47115 because of the backward compatibility issue described in detail in PR #47114.

The new implementation of the BTLDetId class and corresponding BTLNumberingScheme features two numbering schemes that coexist. The first is identical to the old one, and it is used when running simulation in BTL geometry scenarios v2 and v3 (geometry v1 is deprecated and not available anymore), the second is the new BTLDetId, used in v4 geometry, whose features are described in PR #46977. From the geometric layout point of view, v4 is identical to v3, and is used just to cleanly separate the use of old and new numbering scheme.

The two versions of BTLDetIds have two different explicit constructors. A dedicated bit of the BTLDetId itself identify which version of the id is used. All methods implemented to derive the id number of a BTL partition (crystal, sensor module, detector module, readout unit, tray) work in all scenarios. The mapping of these elements are described in two separate DNs for the two cases:
DN-23-006 for geometry v2-v3, and DN-24-014 for v4.

The new numbering scheme also introduces a mapping for the electronic components of BTL, as described in DN-24-014, valid only for the updated version of the BTLDetId and in geometry v4.

PR validation:

All unit tests are functional, with new reference files provided for scenario D110 to account for a minor change in the top level volume in the MTDNumberingBuilder part, and of the revised meaning of RU identifier. Extensive tests have been done on SingleMu samples, showing that the old and new numbering scheme provide consistent results. The BTLDetId ordering affects the digitization, although with no physical meaning and impact, therefore identity should not be expected , just statistical consistency.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 14, 2025

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47353/43694

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @cquarant for master.

It involves the following packages:

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

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

cms-bot commands are listed here

@fabiocos
Copy link
Contributor

please test with cms-data/Geometry-TestReference#20

@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 224KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bbb0d1/44394/summary.html
COMMIT: a3f1421
CMSSW: CMSSW_15_1_X_2025-02-14-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/47353/44394/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

@@ -1557,6 +1585,7 @@
("O9","T39","C19","M14","F8","I17") : "D114",
("O9","T35","C20","M14","F8","I17") : "D115",
("O10","T35","C25","M15","F9","I17") : "D116",
("O9","T35","C18","M14","F8","I18") : "D117",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change it to "O10","T35","C25","M15","F9","I18"
This has removed many overlaps which were in D110. Otherwise it is similar to D110

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated to ("O10","T35","C25","M15","F9","I18") : "D117"

@@ -182,4 +183,5 @@ Several detector combinations have been generated:
* D114 = T39+C19+M11+I17+O9+F8
* D115 = T35+C20+M11+I17+O9+F8
* D116 = T35+C25+M12+I17+O10+F9
* D117 = T35+C18+M11+I18+O9+F8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change it to
D116 = T35+C25+M12+I18+O10+F9

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to D117 = T35+C25+M12+I18+O10+F9

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bsunanda @cquarant I see an inconsistency in between the README and the dictionary for muons: do we want M12 or M15? As far as I can see, the inconsistency is already there for previous scenarios

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, the README muon part seems outdated, as now I see from M13 onwards only in the dictionary

<Include ref='Geometry/MTDSimData/data/v5/mtdProdCuts.xml'/>
<Include ref='Geometry/CMSCommonData/data/FieldParameters.xml'/>
</IncludeSection>
</DDDefinition>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be remade after the changes in Configuration/Geometry/python/dictRun4Geometry.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remade

'Geometry/CMSCommonData/data/FieldParameters.xml',
),
rootNodeName = cms.string('cms:OCMS')
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be remade after the changes in Configuration/Geometry/python/dictRun4Geometry.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remade

@cmsbuild
Copy link
Contributor

Pull request #47353 was updated. @AdrianoDee, @antoniovilela, @cmsbuild, @davidlange6, @DickyChant, @fabiocos, @mandrenguyen, @miquork, @rappoccio can you please check and sign again.

@fabiocos
Copy link
Contributor

please test workflow 32834.0 with cms-data/Geometry-TestReference#20

@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 20KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bbb0d1/44746/summary.html
COMMIT: eea2338
CMSSW: CMSSW_15_1_X_2025-02-28-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/47353/44746/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 16 lines to the logs
  • Reco comparison results: 593 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 4148390
  • DQMHistoTests: Total failures: 2671
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 4145699
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
  • Checked 222 log files, 194 edm output root files, 51 DQM output files
  • TriggerResults: found differences in 1 / 49 workflows

@bsunanda
Copy link
Contributor

bsunanda commented Mar 4, 2025

@antoniovilela,@mandrenguyen, Please merge this PR. I can then submit the PR with correct documentation

@fabiocos
Copy link
Contributor

fabiocos commented Mar 6, 2025

@cms-sw/pdmv-l2 @cms-sw/orp-l2 we need this code integrated to move forward with other developments. Is there any residual concern, comment, or can we move forward with it?

@AdrianoDee
Copy link
Contributor

+pdmv
(sorry I missed it in my queue)

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 6, 2025

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 now be reviewed by the release team before it's merged. @rappoccio, @mandrenguyen, @antoniovilela, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2)
Notice This PR was tested with additional Pull Request(s), please also merge them if necessary: cms-data/Geometry-TestReference#20

fabiocos added a commit to fabiocos/cmssw that referenced this pull request Mar 6, 2025
fabiocos added a commit to fabiocos/cmssw that referenced this pull request Mar 6, 2025
fabiocos added a commit to fabiocos/cmssw that referenced this pull request Mar 6, 2025
…Topology for ETL independent on BTL scenario
@mandrenguyen
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 49423c2 into cms-sw:master Mar 7, 2025
11 checks passed
fabiocos added a commit to fabiocos/cmssw that referenced this pull request Mar 8, 2025
…Topology for ETL independent on BTL scenario
fabiocos added a commit to fabiocos/cmssw that referenced this pull request Mar 8, 2025
commit d34a009
Author: Fabio Cossutti <[email protected]>
Date:   Sat Mar 8 19:05:21 2025 +0100

    Remove update of geometry configuration README, as it is separately done in cms-sw#47531

commit 4bc544a
Author: Fabio Cossutti <[email protected]>
Date:   Thu Mar 6 17:51:14 2025 +0100

    Move to scenarios D118 and D119, rebasing onto cms-sw#47353, make MTDTopology for ETL independent on BTL scenario

commit 91cc88c
Author: Leonardo Lanteri <[email protected]>
Date:   Wed Mar 5 16:56:28 2025 +0100

    added v9/v10 with a working navigation algorithm

commit fa6593d
Author: Leonardo Lanteri <[email protected]>
Date:   Fri Feb 21 10:57:29 2025 +0100

    updated etl/v10/etl.xml with the vv1.7 of etl geometry

commit a5f73dc
Author: Fabio Cossutti <[email protected]>
Date:   Tue Feb 18 17:04:03 2025 +0100

    Add ETL v9/v10 on top of BTL v4

Co-authored-by: Leonardo Lanteri <[email protected]>
fabiocos added a commit to fabiocos/cmssw that referenced this pull request Mar 10, 2025
commit d34a009
Author: Fabio Cossutti <[email protected]>
Date:   Sat Mar 8 19:05:21 2025 +0100

    Remove update of geometry configuration README, as it is separately done in cms-sw#47531

commit 4bc544a
Author: Fabio Cossutti <[email protected]>
Date:   Thu Mar 6 17:51:14 2025 +0100

    Move to scenarios D118 and D119, rebasing onto cms-sw#47353, make MTDTopology for ETL independent on BTL scenario

commit 91cc88c
Author: Leonardo Lanteri <[email protected]>
Date:   Wed Mar 5 16:56:28 2025 +0100

    added v9/v10 with a working navigation algorithm

commit fa6593d
Author: Leonardo Lanteri <[email protected]>
Date:   Fri Feb 21 10:57:29 2025 +0100

    updated etl/v10/etl.xml with the vv1.7 of etl geometry

commit a5f73dc
Author: Fabio Cossutti <[email protected]>
Date:   Tue Feb 18 17:04:03 2025 +0100

    Add ETL v9/v10 on top of BTL v4

Co-authored-by: Leonardo Lanteri <[email protected]>
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.