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: update tests for DD4hep #32111

Merged
merged 3 commits into from
Nov 15, 2020
Merged

Conversation

fabiocos
Copy link
Contributor

@fabiocos fabiocos commented Nov 11, 2020

PR description:

This PR contains a collection of updates to support the validation of DD4hep in standard workflows, beyond the pure dump of geometry objects:

  • update and add mtd-devoted DD4hep ideal configurations based now on the complete ones, commenting all subdetectors but MTD and using an ad-hoc empty tracker mother volume;

  • update the DDD ideal geometry dump, to get back namespaces in the path dump (for easier comparison with DD4hep);

  • add a customization fragment to SimG4Core/PrintGeomInfo to easily use in any configuration the PrintGeomInfo class (the existing code is more an ad-hoc standalone configuration).

PR validation:

By manually adapting the existing DD4hep test workflows to use the MTD-only geometry, the step1 may run, cross MTD (as verified with SteppingVerbosity) and produce SimHits within MTD in MtdSD, as visualized with runSimHitCaloHitDumper_cfg.py.

The issues causing crashes within step1 for Phase2 test workflows are not due to MTD according to this check, they come from the geometry building in DD4hep_DDG4Builder when the whole configuration is used.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32111/19761

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

Geometry/MTDCommonData
SimG4Core/PrintGeomInfo

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88 can you please review it and eventually sign? Thanks.
@makortel, @rovere, @slomeo 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
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 11, 2020

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+1
Tested at: d52310a
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-adfe25/10657/summary.html
CMSSW: CMSSW_11_2_X_2020-11-10-2300
SCRAM_ARCH: slc7_amd64_gcc820

@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-adfe25/10657/summary.html

Comparison Summary:

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

@civanch
Copy link
Contributor

civanch commented Nov 11, 2020

+1

@fabiocos
Copy link
Contributor Author

@kpedro88 @silviodonato @qliphy do you see a problem with this PR?

@kpedro88
Copy link
Contributor

@fabiocos is it necessary to retain non-MTD subdetectors as commented-out lines in the xml, rather than removing them completely?

@fabiocos
Copy link
Contributor Author

@kpedro88 this is for simplicity to compare in a straightforward way with the original from which this is derived. It is not strictly speaking necessary, it makes the comparison a bit simpler, and as it is a test configuration that will go as soon as dd4hep migration is completed I do not think it should create problems

@kpedro88
Copy link
Contributor

+upgrade

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

@silviodonato
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit f41a46d into cms-sw:master Nov 15, 2020
@fabiocos fabiocos deleted the fg-g4geom-cust branch January 8, 2021 08:37
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.

5 participants