MTD geometry/reconstruction: update tests, invert order of forward DetLayers

[fabiocos]

PR description:

This PR addresses an issue reported by @parbol while validating the rebase of the addition of MTD to tracker navigation. As a default the order of the forward DetLayers looks inverted compared to what expected as default ordering (from negative to positive). This PR simply swaps the order, so far it should be irrelevant for the use of the MTD code inside the reconstruction, but matters when adding MTD to the layers navigation.

At present the layers order matters only in the selection of ETL hits and their use for track building inside the TrackExtenderWithMTD, where it was not properly managed, as the code was developed in a single-ETL disc content.
In order to 1) get the correct behaviour that 2) makes the algorithm independent on the input ordering, a corresponding update of the ETL hits search is made (please note that the track build code still needs an update to properly use the timing information of both disks if present, work to follow and be based on this PR).

At the same time geometry tests are updated, also in order to become independent from MTDTopology, whose real usefulness is under scrutiny and will be likely dropped or seriously revised in next future.

PR validation:

Code compiles and run, the local test shows a change in the ordering of DetLayers:

*** allBTLLayers():              1
  0  subdet = Phase2TimingBarrel Barrel = 1 Forward = 0  Cylinder of radius:     117.629585

*** allETLLayers():              4
  0  subdet = Phase2TimingEndcap Barrel = 0 Forward = 1  Disk at:    -303.154999
  1  subdet = Phase2TimingEndcap Barrel = 0 Forward = 1  Disk at:    -300.265015
  2  subdet = Phase2TimingEndcap Barrel = 0 Forward = 1  Disk at:     300.265015
  3  subdet = Phase2TimingEndcap Barrel = 0 Forward = 1  Disk at:     303.154999

*** allForwardLayers():              2
  0  subdet = Phase2TimingEndcap Barrel = 0 Forward = 1  Disk at:     300.265015
  1  subdet = Phase2TimingEndcap Barrel = 0 Forward = 1  Disk at:     303.154999

*** allBackwardLayers():              2
  0  subdet = Phase2TimingEndcap Barrel = 0 Forward = 1  Disk at:    -300.265015
  1  subdet = Phase2TimingEndcap Barrel = 0 Forward = 1  Disk at:    -303.154999

*** allLayers():              5
  0  subdet = Phase2TimingEndcap Barrel = 0 Forward = 1  Disk at:    -303.154999
  1  subdet = Phase2TimingEndcap Barrel = 0 Forward = 1  Disk at:    -300.265015
  2  subdet = Phase2TimingBarrel Barrel = 1 Forward = 0  Cylinder of radius:     117.629585
  3  subdet = Phase2TimingEndcap Barrel = 0 Forward = 1  Disk at:     300.265015
  4  subdet = Phase2TimingEndcap Barrel = 0 Forward = 1  Disk at:     303.154999

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33181/21591

• This PR adds an extra 20KB to repository

[cmsbuild]

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

It involves the following packages:

Geometry/MTDGeometryBuilder
RecoMTD/DetLayers

@perrotta, @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @srimanob, @kpedro88, @slava77, @jpata can you please review it and eventually sign? Thanks.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

[fabiocos]

please test

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d610c9/13516/summary.html
COMMIT: 6a9ede7
CMSSW: CMSSW_11_3_X_2021-03-14-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33181/13516/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• No significant changes to the logs found
• Reco comparison results: 84 differences found in the comparisons
• DQMHistoTests: Total files compared: 37
• DQMHistoTests: Total histograms compared: 2635087
• DQMHistoTests: Total failures: 515
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2634550
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
• Checked 155 log files, 37 edm output root files, 37 DQM output files

[fabiocos]

hold

pending a clarification about the observed differences

[cmsbuild]

Pull request has been put on hold by @fabiocos
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

[fabiocos]

rebased on CMSSW_11_3_X_2021-03-16-2300, benefiting from #33139

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33181/21629

• This PR adds an extra 20KB to repository

[fabiocos]

I have updated the PR description, the bug fixed by #33139 was partly hiding the fact that the order of layers effectively matters in the use of multiple ETL hits for track building (never adjusted so far). Using the appropriate -t 0.99999 threshold for statistical comparison in RelMon, as done by the cms-bot, this is visible also in a standalone test.

With the latest commit anyway the TrackExtenderWithMTD code becomes independent on the order of the endcap layers (verified with printouts and DQM comparison adding the latest commit on both this PR and on top of the original code).

[fabiocos]

please test

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d610c9/13569/summary.html
COMMIT: bc4619b
CMSSW: CMSSW_11_3_X_2021-03-16-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33181/13569/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• No significant changes to the logs found
• Reco comparison results: 128 differences found in the comparisons
• DQMHistoTests: Total files compared: 37
• DQMHistoTests: Total histograms compared: 2639881
• DQMHistoTests: Total failures: 783
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2639076
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
• Checked 155 log files, 37 edm output root files, 37 DQM output files
• TriggerResults: no differences found

[fabiocos]

unhold

[cvuosalo]

+1

[perrotta]

@fabiocos from your comments I derive that you verified the outputs, and understood and approved all differences observed there in the relevant phase2 geometry: could you please confirm?

[fabiocos]

@perrotta the updated test clearly shows that now forward layers are those at positive z, as usually expected. Furthermore, a look at the behaviour of the updated TrackExtenderWithMTD running it on top of both the old and new order of DetLayers shows no more difference, since the protection now ensures that the correct order of the hits is used.

Since previously the use of hits in the extender was depending on the order, some difference has to be expected, and in the premixing samples, where the multiplicity is much higher, they are magnified, this is my interpretation of the DQM output.

[perrotta]

+1

• Fix verified to act as expected in the MTD reco outputs

[fabiocos]

@srimanob comments?

[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)

[qliphy]

+1