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 migration to DD4hep: test configuration and basic test analyzers #28412

Merged
merged 18 commits into from Nov 19, 2019

Conversation

fabiocos
Copy link
Contributor

PR description:

Initial development to migrate the MTD geometry description to the DD4hep backend. In this PR:

  • a new phase 2 scenario D50 is introduced to reorganize the geometry components configuration in a cleaner and more standard way, both for the old and new backend. The geometry description obtained in this way is equivalent to D46 (last MTD geometry cleaning) and should serve also as the baseline for the integration of the new ETL description based on the MTD TDR. Materials are reorganized but the definition is kept unchanged for the time being;

  • the initial DD4hep test configuration provided by @ianna is refreshed to have it fully operational, and a tracker mother volume is defined to avoid all the internal tracker complications. This volume has been verified to be equal to the one of D47;

  • the two test analyzers TestMTDNumbering and TestMTDPosition have been rewritten for DD4hep, and the output compared with the original versions based on DetectorDescription. The numbering is equal (outputs differ only for the use of namespaces in the legacy volume paths), the reference positions appear identical in BTL, with the only differences being in rototranslation dumps:

< Translation vector and Rotation components: ,   1173.3774,     79.9841,     17.9438,     -0.1199,     -0.0000,      0.9928,      0.9928,     -0.0000,      0.1199,     -0.0000,      1.0000,     -0.0000
---
> Translation vector and Rotation components: ,   1173.3774,     79.9841,     17.9437,     -0.1199,     -0.0000,      0.9928,      0.9928,     -0.0000,      0.1199,     -0.0000,      1.0000,     -0.0000

while in ETL some reference positions appear different due to an issue with translations (z coordinate of ETL, to be further investigated):

< Translation vector and Rotation components: ,    313.5000,      0.0000,   3043.5000,      0.0000,     -1.0000,      0.0000,      1.0000,      0.0000,     -0.0000,     -0.0000,      0.0000,      1.0000
< Center global   =     313.500000      0.000000   3043.500000 r =     313.500000 phi =       0.000000
---
> Translation vector and Rotation components: ,    313.5000,     -0.0000,   3017.8500,     -0.0000,     -1.0000,     -0.0000,      1.0000,      0.0000,     -0.0000,     -0.0000,      0.0000,      1.0000
> Center global   =     313.500000     -0.000000   3017.850000 r =     313.500000 phi =      -0.000000
24c24
< Corner 1 global =     266.000000     24.500000   3043.650000 DeltaR =      53.446445
---
> Corner 1 global =     266.000000     24.500000   3018.000000 DeltaR =      53.446445

This code serves as a basis for any further debugging/development.

PR validation:

Code compiles and run, analyzers produce output that can be compared with the older versions.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28412/12783

  • This PR adds an extra 84KB to repository

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

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@fabiocos
Copy link
Contributor Author

code-checks

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28412/12785

  • This PR adds an extra 216KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

Configuration/Geometry
Geometry/CMSCommonData
Geometry/MTDCommonData
Geometry/MTDSimData

@civanch, @Dr15Jones, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88 can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @vargasa this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@fabiocos
Copy link
Contributor Author

@pmeridian @parbol FYI

@fabiocos
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 16, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3503/console Started: 2019/11/16 19:41

@cmsbuild
Copy link
Contributor

-1

Tested at: 8de4fbc

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-aa258c/3503/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests

  • Unit Tests:

I found errors in the following unit tests:

---> test test2026Geometry had ERRORS

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Pull request #28412 was updated. @civanch, @Dr15Jones, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @franzoni, @kpedro88, @fabiocos, @davidlange6 can you please check and sign again.

@fabiocos
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 18, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3521/console Started: 2019/11/18 17:08

@fabiocos
Copy link
Contributor Author

@ianna @kpedro88 I consider this PR as complete for its purpose, i.e. to have a testbed for the MTD geometry in DD4hep to start with. Scenario D50 should be considered as a checkpoint to technically compare DetectorDescription and DD4hep implementations, using a new organization for data files, but it is not adding any new description, so I would not push to have a complete set of relvals corresponding to it (as they should not provide anything different wrt D46). This will be the basis for the initial implementation of the new ETL design (xml almost ready to be integrated, numbering scheme to be setup).

@ianna the latest updates after your last signature are merely technical, they do not change the geometrical description of the detector

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-aa258c/3521/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2936360
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2936018
  • DQMHistoTests: Total skipped: 341
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@ianna
Copy link
Contributor

ianna commented Nov 18, 2019

+1

@kpedro88
Copy link
Contributor

+upgrade

@fabiocos
Copy link
Contributor Author

+operations

@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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@fabiocos
Copy link
Contributor Author

+1

@cmsbuild cmsbuild merged commit 5bfb0ea into cms-sw:master Nov 19, 2019
@fabiocos fabiocos deleted the fc-mtd-dd4hep branch November 20, 2019 13:24
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

5 participants