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: clean BTLDetId #38589

Merged
merged 4 commits into from Jul 11, 2022
Merged

Conversation

fabiocos
Copy link
Contributor

@fabiocos fabiocos commented Jul 4, 2022

PR description:

In preparation of the update of the BTL geometry, this PR cleans the structure of BTLDetId by removing unused methods and support for old scenarios no more present since years in the release. The handling of multiple geometry scenarios for both BTL and ETL is improved by adding to ETLDetId a EtlLayout enum class, similar to BTLDetId::CrysLayout, and by allowing the MTDTopologyMode class to separately provide the BTL and ETL layout values depending on the global topology mode value.

The code exploiting this piece of information in geometry, reconstruction and validation classes is updated accordingly.
A new BTL tdr mode is added in preparation of the new geometry. While the crystals layout should stay the same, the logical grouping into modules should change wrt to barphiflat, moving from a 16 x 3 matrix, as in the original tkLayout based BTL xml geometry implementation, to a 16 crystals array per module. This will directly impact the BTLDetId definition, which is based on the volumes stack and their copy number.

PR validation:

The unit tests in MTD geometry and RecoMTD/DetLayers packages show that the existing geometry definition stays unchanged

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 4, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38589/30846

  • This PR adds an extra 60KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 4, 2022

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

It involves the following packages:

  • DataFormats/ForwardDetId (upgrade, simulation)
  • Geometry/MTDCommonData (geometry, upgrade)
  • Geometry/MTDGeometryBuilder (geometry, upgrade)
  • RecoLocalFastTime/FTLCommonAlgos (upgrade, reconstruction)
  • RecoMTD/DetLayers (upgrade, reconstruction)
  • Validation/MtdValidation (dqm)

@civanch, @Dr15Jones, @clacaputo, @makortel, @cvuosalo, @emanueleusai, @ianna, @mdhildreth, @cmsbuild, @AdrianoDee, @srimanob, @jfernan2, @ahmad3213, @jpata, @pmandrik, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks.
@trtomei, @apsallid, @beaucero, @rovere, @bsunanda this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@fabiocos
Copy link
Contributor Author

fabiocos commented Jul 4, 2022

please test

@fabiocos
Copy link
Contributor Author

fabiocos commented Jul 4, 2022

@casarsa @gsorrentino18 FYI

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 4, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5899fa/25970/summary.html
COMMIT: 552d172
CMSSW: CMSSW_12_5_X_2022-07-04-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38589/25970/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: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3654771
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3654741
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@fabiocos
Copy link
Contributor Author

fabiocos commented Jul 5, 2022

Failures in comparisons are unrelated to this PR

@civanch
Copy link
Contributor

civanch commented Jul 5, 2022

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 5, 2022

Pull request #38589 was updated. @civanch, @Dr15Jones, @clacaputo, @makortel, @cvuosalo, @emanueleusai, @ianna, @mdhildreth, @cmsbuild, @AdrianoDee, @srimanob, @jfernan2, @ahmad3213, @jpata, @pmandrik, @micsucmed, @rvenditti can you please check and sign again.

@fabiocos
Copy link
Contributor Author

fabiocos commented Jul 5, 2022

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 5, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5899fa/25986/summary.html
COMMIT: 0dc8e58
CMSSW: CMSSW_12_5_X_2022-07-05-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38589/25986/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: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3654771
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3654747
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@civanch
Copy link
Contributor

civanch commented Jul 6, 2022

+1

@emanueleusai
Copy link
Member

+1

@clacaputo
Copy link
Contributor

+reconstruction

@srimanob
Copy link
Contributor

+Upgrade

This PR can consider to be a technical PR to clean up obsolete MTD methods which are not used anymore in the current release. No change is expected from exisiting Phase-2 geometry.

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

@perrotta
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 3905153 into cms-sw:master Jul 11, 2022
@fabiocos fabiocos deleted the fc-cleanBTLDetId branch July 19, 2022 13:02
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.

None yet

7 participants