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 - Numbering scheme update #46977

Merged
merged 5 commits into from
Dec 20, 2024

Conversation

cquarant
Copy link
Contributor

PR description:

Implementation of new numbering scheme for the BTLDetId.
ID based on the geometry v3 of BTL: only one type of crystals (same thickness), instead of the three types of v2.
It also better describe the modularity of BTL, allowing to identify each component of the detector in its final design.

Geometry v1 is considered obsolete and it's not supported.

The code is backward compatible with Geometry v2: simulated events produced in v2 are automatically translated in the new format.

The code also implements a mapping to identify the electronic BTL components and the two electronic channels corresponding to a crystal, specifying also the side of the crystal they're connected to.

The new numbering scheme and the electronics mapping are documented in these slides (https://indico.cern.ch/event/1485592/#6-btl-numering-scheme-update-a) and in Detector Note 2024/14 (https://icms.cern.ch/tools/publications/notes/entries/DN/2024/014)

This PR reverts commit 06d30e1 and activates the new order in BTL reconstruction geometry from -pi to pi, aligning the order of BTL modules in MTD geometry and the tracking navigation geometry in RecoMTD/DetLayers.

It is integrated on top of #46911 , and it requires the update of the unit test references. This update is proposed in parallel to the update of the BTL numbering scheme, that also requires the update of all references.

PR validation:

The following tests has been run:

  • Geometry/MTDCommonData/test/TestMTDIdealGeometry.cc
  • RecoMTD/DetLayers/test/MTDRecoGeometryAnalyzer.cc
  • Geometry/MTDGeometryBuilder/test/MTDDigiGeometryAnalyzer.cc
  • Geometry/MTDNumberingBuilder/test/GeometricTimingDetAnalyzer.cc

The result with the test is coherent with the new numbering, reference file for unit tests (https://github.com/[cms-data/Geometry-TestReference](https://github.com/cms-data/Geometry-TestReference)) needs to be updated.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 17, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46977/43056

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • DataFormats/ForwardDetId (upgrade, simulation)
  • DataFormats/TrackReco (reconstruction)
  • Geometry/MTDCommonData (geometry, upgrade)
  • Geometry/MTDGeometryBuilder (geometry, upgrade)
  • Geometry/MTDNumberingBuilder (geometry, upgrade)
  • RecoMTD/DetLayers (reconstruction, upgrade)

@Dr15Jones, @Moanwar, @bsunanda, @civanch, @cmsbuild, @jfernan2, @kpedro88, @makortel, @mandrenguyen, @mdhildreth, @srimanob, @subirsarkar can you please review it and eventually sign? Thanks.
@VinInn, @VourMa, @apsallid, @bsunanda, @denizsun, @fabiocos, @gpetruc, @martinamalberti, @missirol, @mmusich, @mtosi, @rovere, @salimcerci, @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#18

@jfernan2
Copy link
Contributor

+1

@Moanwar
Copy link
Contributor

Moanwar commented Dec 18, 2024

+Upgrade

@fabiocos
Copy link
Contributor

@cms-sw/geometry-l2 @cms-sw/simulation-l2 comments or questions?

@civanch
Copy link
Contributor

civanch commented Dec 20, 2024

+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 now be reviewed by the release team before it's merged. @rappoccio, @mandrenguyen, @sextonkennedy, @antoniovilela (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#18

@mandrenguyen
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 84cbe95 into cms-sw:master Dec 20, 2024
11 checks passed
@mmusich
Copy link
Contributor

mmusich commented Dec 20, 2024

@mandrenguyen it looks like after this PR was merged (without the corresponding cms-data update cms-data/Geometry-TestReference#18), we're experiencing unit tests failures in PR tests, see e.g.: here

@fabiocos
Copy link
Contributor

the update in the reference is indeed necessary to have all unit tests properly working, without merging cms-data/Geometry-TestReference#18 failures have to be expected otherwise

@fabiocos
Copy link
Contributor

@cms-sw/orp-l2 Unfortunately I realize that this PR is breaking backward compatibility, despite the schema evolution being correctly managed for BTLDetId: whenever a detid is save as a uint32_t member of a class, this is potentially never translated. In the move between steps (step1 -> step2 and step2 -> step3) that @cquarant tested in detail, and in reading explictly BTLDetIds, things are working due to the way the information is stored and read, as far as I can see. But a simple standalone validation code run shows the issue.

In order to prevent breaking CMSSW_15_0_0_pre2, the short term solution is to revert this PR, and in parallel the cms-data PR for references used in unit tests. Then I see at present two possible solutions:

  • apply schema evolution to all collections containing a BTL det id as uint32_t, forcing the translation (there are several of them, but not a huge number);
  • introduce a new geometry, that is fake in the sense that it is identical to the current one, but for the numbering scheme and some volume naming variation needed to differentiate the schemes; in this way we might force the association of numbering scheme to a scenario, and force the solution of the problem.

Both approaches seem ugly to me, but I do not see others at present: @makortel do you have any suggestion? In any case this should be done not in a hurry, after pre2 is built.

@makortel
Copy link
Contributor

Is it important to be able to read (re-interpret) the old files?

@fabiocos
Copy link
Contributor

@makortel I would love to forget about them, but it is not the policy we have always used in my understanding. It would mean that from CMSSW_15_0_0_pre2 we are unable to correctly associate to the BTL geometry any collection including a BTLDetId in the raw form of uint32_t made up to 15_0_0_pre1. A simple example is in

https://github.com/cms-sw/cmssw/blob/master/Validation/MtdValidation/test/mtdValidation_cfg.py#L78

where we are rereconstructing tracking rec hits from clusters, failing in

https://github.com/cms-sw/cmssw/blob/master/RecoLocalFastTime/FTLRecProducers/plugins/MTDTrackingRecHitProducer.cc#L91

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