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

[DD4hep] Increase precision of rotation matching in making DD4hep big XML file for DB #35299

Merged
merged 1 commit into from Sep 17, 2021

Conversation

cvuosalo
Copy link
Contributor

The extended geometry description (also called the big XML file) stored in the DB specifies a rotation for each physical volume and boolean solid, though the rotation can be omitted if it is the identity rotation. There is a long list of rotations, and many rotations are defined multiple times with different names. When the big XML file is generated, particular rotation names have to be chosen. Prior to this PR, matching was done with insufficient precision so that very close but different rotations were confused. In particular, the rotation eealgo:EECrRoC1R1, which is a 1e-6 radian rotation, was confused with the identity rotation. This problem was compounded by DD4hep ignoring this tiny rotation during volume placement and treating it as identity. The result was that, with the DD4hep geometry from the DB, some volumes were slightly rotated incorrectly, which caused small overlaps.

This PR increases the precision of rotation matching during creation of the big XML file, so different rotations that are very close will not be confused. It needs to go along with a fix in DD4hep that will allow DD4hep to recognize the tiny eealgo:EECrRoC1R1 rotation.

All the rotations in the geometry system were checked, and eealgo:EECrRoC1R1 was found to be the only one so small that it was close to identity.

This PR also implements following the order of rotation definitions in the source XML files, so when the same rotation is defined with different names, the first name is used. This method favors the standard rotation names in rotations.xml over the redefinitions in sub-detector XML files.

Note that this PR only changes the tool that creates the big XML file and does not directly affect any workflow.

PR validation:

The DD4hep big XML file was created and compared carefully against the DDD big XML file. Sunanda has already shown that replacing mismatched rotations with the identity rotation eliminates the overlaps of concern.

After the DD4hep fix is merged into CMSSW, the DD4hep big XML file will be created with this PR and will be tested and validated prior to uploading to the Conditions DB.

This PR should be backported to 12_0 to allow creation of a 12_0 DD4hep big XML file if needed.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35299/25309

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @cvuosalo (Carl Vuosalo) for master.

It involves the following packages:

  • DetectorDescription/DDCMS (geometry)
  • DetectorDescription/OfflineDBLoader (geometry)

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild can you please review it and eventually sign? Thanks.
@fabiocos, @slomeo, @vargasa 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

@cvuosalo
Copy link
Contributor Author

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-623a46/18646/summary.html
COMMIT: 2fd71aa
CMSSW: CMSSW_12_1_X_2021-09-14-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35299/18646/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: 39
  • DQMHistoTests: Total histograms compared: 3000833
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3000805
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

@civanch
Copy link
Contributor

civanch commented Sep 16, 2021

+1

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

@civanch
Copy link
Contributor

civanch commented Sep 16, 2021

urgent

@qliphy
Copy link
Contributor

qliphy commented Sep 17, 2021

+1

@cmsbuild cmsbuild merged commit e662c63 into cms-sw:master Sep 17, 2021
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

4 participants