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: updated GeometricTimingDet to remove the residual dependency on DDSolidShape #30834

Merged
merged 2 commits into from
Jul 28, 2020

Conversation

fabiocos
Copy link
Contributor

PR description:

This PR is a follow-up of #29905 as mentioned in #28914 (comment) . The residual dependence of GeometricTimingDet on the DDD class DDSolidShape is removed through the provided map, and cms::DDSolidShape is now used. No impact is expected outside of Geometry/MTDNumberingBuilder.

PR validation:

The MTD geometry unit tests are ok, test wf 23234.0 runs smoothly. The full output of the MTDNumberingBuilder test shows the expected change in shape of polycones:

71c71
<  shape = DDSolidShape index:5, name: Polycone_rz
---
>  shape = DDSolidShape index:7, name: Polycone_rrz
127c127
<  shape = DDSolidShape index:5, name: Polycone_rz
---
>  shape = DDSolidShape index:7, name: Polycone_rrz
183c183
<  shape = DDSolidShape index:5, name: Polycone_rz
---
>  shape = DDSolidShape index:7, name: Polycone_rrz

which anyway should have no practical effect on the model built in memory and its use.

@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-30834/17171

  • This PR adds an extra 16KB 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.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30834/17172

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

Geometry/MTDNumberingBuilder

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88 can you please review it and eventually sign? Thanks.
@silviodonato, @dpiparo 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 Jul 20, 2020

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-1

Tested at: abe561e

CMSSW: CMSSW_11_2_X_2020-07-19-2300
SCRAM_ARCH: slc7_amd64_gcc820
You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a1ce91/8124/summary.html

I found follow errors while testing this PR

Failed tests: RelVals

  • RelVals:

When I ran the RelVals I found an error in the following workflows:
1330.0 step5

runTheMatrix-results/1330.0_ZMM_13+ZMM_13+DIGIUP15+RECOUP15_L1TMuDQM+HARVESTUP15_L1TMuDQM+NANOUP15/step5_ZMM_13+ZMM_13+DIGIUP15+RECOUP15_L1TMuDQM+HARVESTUP15_L1TMuDQM+NANOUP15.log

@cmsbuild
Copy link
Contributor

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

@cmsbuild
Copy link
Contributor

+1
Tested at: abe561e
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a1ce91/8157/summary.html
CMSSW: CMSSW_11_2_X_2020-07-20-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-a1ce91/8157/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: 2525996
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2525948
  • DQMHistoTests: Total skipped: 47
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 144 log files, 17 edm output root files, 34 DQM output files

@cvuosalo
Copy link
Contributor

@fabiocos The output you showed is still not quite explained by the code you referenced, since the code line is:

shape = " << det->shape() << "\n\n"

while the output says "shape = DDSolidShape index:5, name: Polycone_rz".
But what I really want to check is that, from previous version to this PR, the enum value goes from 7 -> 5. If the value goes 5-> 7, then something strange is happening.

@fabiocos
Copy link
Contributor Author

@cvuosalo concerning the printout, if you check the implementation of the << operator for solid shapes https://cmssdt.cern.ch/lxr/source/DetectorDescription/Core/src/DDSolid.cc#0030 you see the format shown above.

The output is a diff between the new (5) and the old (7) output, you may easily verify it yourself by running https://cmssdt.cern.ch/lxr/source/Geometry/MTDNumberingBuilder/test/mtd_cfg.py for scenario D53 . Shape n. 7 is no more implemented as a polycone, do you agree?

@cvuosalo
Copy link
Contributor

+1

@cvuosalo
Copy link
Contributor

@fabiocos Thanks for the explanation and thanks for taking care of this issue.

@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

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