Rectangular mtd cleaning

[parbol]

PR description:

This PR proposes the following main changes:

• A cleaning and simplification of the Geometry/MTDGeometryBuilder/interface/RectangularMTDTopology.h class and surrounding classes. The previous code was based on the tracker code and contained many unneeded features for the MTD purposes. These changes by themselves should not affect any output as we have tested locally.

• Cleaning of the MTDTopology class Geometry/MTDNumberingBuilder/interface/MTDTopology.h, which was essentially unused in the code with the exception of a couple of methods. These changes by themselves should not affect any output as we have tested locally.

• We have also inverted the order of the 11th and 12th positions of the ETL vector in mtdParameters.xml in such a way that the 32 pixels are aligned with the long side of the sensor, and the 16 pixels with the short side (It was the other way around before).

• Addition of new information and methods in the RectangularMTDTopology class to include the non-active interpixel gaps for the ETL and the gaps between the pixels and the sensor borders. This 4 parameters (2 for the x coordinate and 2 for the y coordinate) have been added to mtdParameters.xml. The second 4 positions of the ETL parameters vector have been set to 0 as they are not used anywhere else in the code. This change should slightly change the acceptance of the ETL sensors (since small 50 microns gaps should appear in between pixels and between the pixels and the border of the sendor).

• Modifications have been made to Geometry/MTDGeometryBuilder/test/MTDDigiGeometryAnalyzer.cc in order to dump the position of the pixels, and also to SimFastTiming/FastTimingCommon/src/ETLDeviceSim.cc in order to discard simhits landing on the gaps.

PR validation:

• We have checked that the only change in the Digitization is related to the gaps in between the ETL pixels. In this link you can see how the simhits look like with slightly exagerated interpad distances (150 microns in the plots vs. the 50 more realistic gap in the code)

https://cernbox.cern.ch/index.php/s/xweGn88Zs9XN036

• We have checked that the positions of the pixels in the ETL sensors are as expected.

[cmsbuild]

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33340/21914

• This PR adds an extra 36KB to repository

• There are other open Pull requests which might conflict with changes you have proposed:

  • File SimFastTiming/FastTimingCommon/src/ETLDeviceSim.cc modified in PR(s): Migrate most of MixingModule and PreMixingModule to EventSetup consumes #31697

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

• code-format:
  https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33340/21914/code-format.patch
  e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33340/21914/code-format.patch | patch -p1
  You can also run scram build code-format to apply code format directly

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33340/21915

• This PR adds an extra 40KB to repository

• There are other open Pull requests which might conflict with changes you have proposed:

  • File SimFastTiming/FastTimingCommon/src/ETLDeviceSim.cc modified in PR(s): Migrate most of MixingModule and PreMixingModule to EventSetup consumes #31697

[cmsbuild]

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

It involves the following packages:

DataFormats/ForwardDetId
Geometry/MTDCommonData
Geometry/MTDGeometryBuilder
Geometry/MTDNumberingBuilder
SimFastTiming/FastTimingCommon

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @srimanob, @kpedro88 can you please review it and eventually sign? Thanks.
@apsallid, @fabiocos, @rovere this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

[fabiocos]

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

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3085c0/14025/summary.html
COMMIT: 13f2607
CMSSW: CMSSW_11_3_X_2021-04-05-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33340/14025/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS Clang-Tidy warnings: There are Clang-Tidy warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3085c0/14025/llvm-analysis/cmsclangtidy.txt for details.

Comparison Summary

Summary:

• No significant changes to the logs found
• Reco comparison results: 12131 differences found in the comparisons
• DQMHistoTests: Total files compared: 37
• DQMHistoTests: Total histograms compared: 2640868
• DQMHistoTests: Total failures: 129238
• DQMHistoTests: Total nulls: 8
• DQMHistoTests: Total successes: 2511600
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: -0.004 KiB( 36 files compared)
• DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
• Checked 155 log files, 37 edm output root files, 37 DQM output files
• TriggerResults: no differences found

[fabiocos]

@parbol I guess that most of the apparently unrelated differences are linked to the modified use of the same sequence of random numbers in digitizers. Is the output what you expect?

[civanch]

+1

[srimanob]

+Upgrade

[cmsbuild]

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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

[fabiocos]

@silviodonato @qliphy for the integration, this has to go in parallel with cms-data/Geometry-TestReference#6 otherwise one MTD geometry unit test will fail

[qliphy]

+1

[fabiocos]

@parbol the update of the parameters in mtdParameters.xml is done only for the new ETL (D76 and following), but the code using them is applied also to the older scenarios D49 and D60, I do not see any protection against this. With the old default parameters, I guess we are getting close to random results, and this should explain the differences observed in #33598 test, do you agree? Setting all the first eight parameters to 0 is the way to preserve the old behaviour I would say.

[parbol]

Hi Fabio,

Yes, this makes sense. I will update it.