[DD4hep] Tool for creation of DD4hep geometry DB payloads and for migration validation #34111

cvuosalo · 2021-06-14T06:38:11Z

There is a tool for expert use to create geometry DB payloads from DDD XML files. This PR provides a DD4hep version of the same tool. An initial decision was made to match the design and function of the DDD tool to reduce development risks and to facilitate validation.

Note that this tool is intended for the creation of Run 3 DD4hep geometry DB payloads. At this time, it is not anticipated that Runs 1-2 DB geometry payloads will be re-built. For Phase 2, the suitability of this design will have to be assessed, and changes may be necessary.

This tool produces the complete detector geometry in a human-readable XML file, which is not only suitable for loading into the DB but also can be used for diagnostics and validation.

There are two minor issues that will be addressed in follow-up PRs. The DB payload creation script currently has the reduced material scenarios commented out. A subsequent PR will implement those scenarios. Secondly, PPS uses a special XML payload for reconstruction, which is currently created with DDD. A later PR will implement creation of this payload with DD4hep.

Validation
The extended geometry description DB payload created by the tool was validated against its DDD counterpart incrementally, section by section, to ensure the content was identical. A stress test where the tool read its own output was also done to verify it would reproduce that output. Also, 100 ttbar events generated with DD4hep XML and DD4hep DB payloads were checked to verify that there were no significant differences in results. A RelVal is underway for further validation.

Reco geometry DB payloads were checked to see that they are identical to the ones created with DDD. The Tracker Reco geometry DB payload created with DD4hep is not numerically identical to the DDD version, but it was thoroughly checked to see that the content is the same as DDD, except for insignificant numerical variations. The output of DDDCmsTrackerContruction::printAllTrackerGeometricDets was checked, as well as the ModuleInfo.log, ModuleNumbering.dat, TECLayout_CMSSW.dat outputs from Geometry/TrackerGeometryBuilder/test/ModuleInfo.cc, and all were found to be the same for DD4hep and DDD. The only differences were phi angles equal to |pi| that varied between -pi and +pi, and tiny values near 0 that could varied between -1e-12 and +1e-12.

To avoid these variations in future comparisons when new DB payloads are candidates for uploading, this PR proposes that values in the DD4hep Tracker Reco geometry record (the PGeometricDet) be standardized as follows:

phi values of |pi| will be set to +pi.
Translation x, y, z values and rotation matrix elements that are <=|1e-10| will be set to true 0.
This proposal has already been discussed with the Tracker DPG.

DetectorDescription/OfflineDBLoader/plugins/OutputDDToDDL.cc

cmsbuild · 2021-06-14T06:46:32Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34111/23303

This PR adds an extra 56KB to repository
Found files with invalid states:
- DetectorDescription/DDCMS/src/DDParsingContext.cc:
  - Added: 1dc1c16
  - Modified: a9a4d4e
  - Deleted: b70681b
There are other open Pull requests which might conflict with changes you have proposed:
- File DetectorDescription/DDCMS/interface/DDParsingContext.h modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File DetectorDescription/DDCMS/interface/DDXMLTags.h modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File DetectorDescription/DDCMS/plugins/dd4hep/DDDefinitions2Objects.cc modified in PR(s): [DD4hep] start on geometry XML payload producer #33548

cmsbuild · 2021-06-14T06:46:52Z

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

It involves the following packages:

CondTools/Geometry
DetectorDescription/DDCMS
DetectorDescription/OfflineDBLoader

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

cmsbuild · 2021-06-16T05:47:04Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34111/23345

This PR adds an extra 20KB to repository
Found files with invalid states:
- DetectorDescription/DDCMS/src/DDParsingContext.cc:
  - Added: 1dc1c16
  - Modified: a9a4d4e
  - Deleted: b70681b
There are other open Pull requests which might conflict with changes you have proposed:
- File DetectorDescription/DDCMS/interface/DDParsingContext.h modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File DetectorDescription/DDCMS/interface/DDXMLTags.h modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File DetectorDescription/DDCMS/plugins/dd4hep/DDDefinitions2Objects.cc modified in PR(s): [DD4hep] start on geometry XML payload producer #33548

cmsbuild · 2021-06-16T05:47:26Z

Pull request #34111 was updated. @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @ggovi can you please check and sign again.

ianna

@cvuosalo - IMHO, the whole approach is seriously flawed. It tries to re-implement the DDD Stores, but does it half way. It uses a full Geant4 geometry where a simpler and more elegant approach would do (e.g. DDD Graph).

@makortel - FYI

ianna · 2021-06-16T05:59:06Z

DetectorDescription/OfflineDBLoader/plugins/OutputDD4hepToDDL.cc

+#include "G4Material.hh"
+#include "G4ReflectionFactory.hh"
+#include "TGeoManager.h"
+


@cvuosalo - it looks like a very complicated way to make one XML file out of a few XML files. You introduce dependency on ROOT, DD4hep, AND Geant4 to do that! A much more elegant and efficient way is to simply use DDD that creates a Graph.

ianna · 2021-06-16T06:04:25Z

DetectorDescription/DDCMS/src/DDDetector.cc

@@ -19,6 +20,8 @@ namespace cms {

    m_description = &dd4hep::Detector::getInstance(tag);
    m_description->addExtension<cms::DDVectorsMap>(&m_vectors);
+    m_context = new cms::DDParsingContext(*m_description, makePayload);


@cvuosalo - is it a memory leak?

See below on line 34. In general use, when this code is not creating a DB payload, the parsing context is removed after the end of parsing. If I understand DD4hep correctly, the removeExtension method issues a delete on the pointer that was passed to addExtension.

Please, do not expose an XML parsing context beyond an XML parser. Thanks.

cvuosalo · 2021-06-16T16:26:18Z

Refinement and simplification is certainly possible as time allows. My first priority was to try to match the old DD DB payload as closely as possible. This whole design will need to be revisited for Phase 2 DD4hep, which has more challenging requirements than Run 3. I think Phase 2 is a good opportunity for considering new approaches for the DB payloads, since the DB payloads will be completely new. Some of the code in this PR might be useful for Phase 2, or maybe it will be dropped for an entirely different method.

ianna · 2021-06-17T05:55:19Z

Refinement and simplification is certainly possible as time allows. My first priority was to try to match the old DD DB payload as closely as possible. This whole design will need to be revisited for Phase 2 DD4hep, which has more challenging requirements than Run 3. I think Phase 2 is a good opportunity for considering new approaches for the DB payloads, since the DB payloads will be completely new. Some of the code in this PR might be useful for Phase 2, or maybe it will be dropped for an entirely different method.

The problem with this temporary solution is that an Assembly as an XML description is ignored. Once a payload is produced using this approach, we’d have to support it. This has a long time implication and restricts the future design. I’d suggest we do not use a DB payload for an ideal geometry description in DD4hep-based workflows until we have a proper solution.

cmsbuild · 2021-06-17T06:30:45Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34111/23369

This PR adds an extra 16KB to repository
Found files with invalid states:
- DetectorDescription/DDCMS/src/DDParsingContext.cc:
  - Added: 1dc1c16
  - Modified: a9a4d4e
  - Deleted: b70681b
There are other open Pull requests which might conflict with changes you have proposed:
- File DetectorDescription/DDCMS/interface/DDParsingContext.h modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File DetectorDescription/DDCMS/interface/DDXMLTags.h modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File DetectorDescription/DDCMS/plugins/dd4hep/DDDefinitions2Objects.cc modified in PR(s): [DD4hep] start on geometry XML payload producer #33548

cmsbuild · 2021-06-17T06:31:08Z

Pull request #34111 was updated. @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @ggovi can you please check and sign again.

ianna

some more questions

ianna · 2021-06-17T12:46:08Z

DetectorDescription/DDCMS/interface/DDXMLTags.h

@@ -137,6 +137,7 @@ namespace cms {
  UNICODE(rParent);
  UNICODE(ChildName);
  UNICODE(rChild);
+  UNICODE(rPhysVol);


@cvuosalo - why is it needed?

please, remove

ianna · 2021-06-17T12:48:36Z

DetectorDescription/DDCMS/plugins/dd4hep/DDDefinitions2Objects.cc

@@ -860,6 +865,16 @@ void Converter<DDLPosPart>::operator()(xml_h element) const {
  string childName = ns.prepend(ns.attr<string>(e.child(DD_CMU(rChild)), _U(name)));
  Volume parent = ns.volume(parentName, false);
  Volume child = ns.volume(childName, false);
+  xml_dim_t physVolName = e.child(DD_CMU(rPhysVol), false);


@cvuosalo - why not use to (re-)store an Assembly?

Yes, this tag was my workaround since Assembly had no definition in XML until yesterday. But now that we can define assemblies in XML, this workaround can be dropped and replaced with explicit Assembly definitions, which are much more elegant.

cvuosalo · 2021-07-25T21:09:22Z

The unit test errors appear to be unrelated to this PR and seem to be in the IB and related to:
#34601
#34587

cvuosalo · 2021-07-25T21:11:40Z

The Event Setup warnings in CondTools/Geometry/plugins/PGeometricDetBuilder.cc should be resolved in a separate PR. The DB group may already be working on this as part of the ES migration to tokens.

cvuosalo · 2021-07-26T16:21:29Z

@cms-sw/db-l2 Could you please review this PR soon? I would like to try to include it in 12_0_0_pre5 that is due July 27th. Thanks.

cvuosalo · 2021-07-26T20:56:40Z

@cmsbuild please test with #34622

cmsbuild · 2021-07-27T02:05:23Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-fb9c5d/17217/summary.html
COMMIT: a9b3ef1
CMSSW: CMSSW_12_0_X_2021-07-26-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/34111/17217/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 3 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-fb9c5d/17217/llvm-analysis/esrget-sa.txt for details.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 2998564
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2998535
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 · 2021-07-27T07:11:39Z

+1

ggovi · 2021-07-27T07:12:34Z

+1

cmsbuild · 2021-07-27T07:13:00Z

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

civanch · 2021-07-27T10:24:45Z

urgent

ianna

@cvuosalo - I still have a number of concerns about this code. We should discuss it offline.

ianna · 2021-07-27T11:02:53Z

CondTools/Geometry/test/writehelpers/createExtended2021DD4hepPayloads.sh

+# reco parts of the database are also filled.
+cmsRun geometryExtended2021DD4hep_writer.py
+
+# Now put the other scenarios into the database.


@cvuosalo - please, remove commented out code.

As mentioned in the PR description, a forthcoming PR will enable these lines to produce the reduced material scenarios. Right now the lines serve as reminders of what will be implemented soon.

ianna · 2021-07-27T11:29:26Z

Geometry/TrackerNumberingBuilder/src/GeometricDet.cc

@@ -112,6 +114,12 @@ GeometricDet::GeometricDet(cms::DDFilteredView* fv, GeometricEnumType type)
      shape_(fv->shape()),
      params_(computeLegacyShapeParameters(shape_, fv->solid())),
      isFromDD4hep_(true) {
+  using namespace angle_units::operators;
+  if (almostEqual(phi_, -1._pi, 10)) {


@cvuosalo - how much does it cost in terms of performance loss?

@ianna , I agree with your concerns but just today we should decide to merge this PR or to close it. We cannot improve it anymore.

I would propose to merge and for the next release 12_1_0 we should put efforts for the code optimization.

These lines are executed when the GeometricDet is being built with DD4hep from XML. In production, the GeometricDet is read from the PGeometricDet in the DB, so this constructor is not called.

A quick check of the start-up time (including XML processing) of DD4hep XML workflow 11634.911 shows no significant difference between this PR and another PR test without this PR. More careful checks will be needed to determine whether this added function has a measurable effect on performance. Note that the almostEqual function is a simple template, so it shouldn't have much overhead.

These lines are executed when the GeometricDet is being built with DD4hep from XML. In production, the GeometricDet is read from the PGeometricDet in the DB, so this constructor is not called.

This is not true for the Phase 2 geometry that is using XML as a main source.

More careful checks will be needed to determine whether this added function has a measurable effect on performance. Note that the almostEqual function is a simple template, so it shouldn't have much overhead.

Yes, careful performance measurement ought to be done before merging the code. Any operation times the number of GeometricDets will have an effect.

Performance testing of this if statement shows no significant effect. The PGeometricDetBuilder, which creates the GeometricDet and copies it to a persistent version, was tested with and without this if check. The total user time in both cases was about 14.7 +- 0.2 seconds. It appears that the cost of this if statement is not significant compared to the time required to navigate through the geometry in order to calculate the GeometricDet.
For reference, in Run 3 there are 24508 GeometricDet records, with three cases where a phi value of -pi was calculated and reset to pi by this check.

qliphy · 2021-07-27T16:50:52Z

+1

cmsbuild added this to the CMSSW_12_0_X milestone Jun 14, 2021

cmsbuild added code-checks-pending db-pending geometry-pending orp-pending pending-signatures tests-pending labels Jun 14, 2021

cvuosalo commented Jun 14, 2021

View reviewed changes

DetectorDescription/OfflineDBLoader/plugins/OutputDDToDDL.cc Outdated Show resolved Hide resolved

cmsbuild added code-checks-approved and removed code-checks-pending labels Jun 14, 2021

makortel mentioned this pull request Jun 14, 2021

[DD4hep] Minor size correction of 0.1 to 7 degrees in phi for tube shapes that are non-sensitive TIB pillars and ribs #34088

Merged

cmsbuild added code-checks-pending and removed code-checks-approved labels Jun 16, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Jun 16, 2021

ianna suggested changes Jun 16, 2021

View reviewed changes

cmsbuild mentioned this pull request Jun 16, 2021

[DD4hep] start on geometry XML payload producer #33548

Closed

cmsbuild added code-checks-pending and removed code-checks-approved labels Jun 17, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Jun 17, 2021

ianna suggested changes Jun 17, 2021

View reviewed changes

cmsbuild added requires-external tests-started and removed tests-rejected labels Jul 26, 2021

cmsbuild added tests-approved and removed tests-started labels Jul 27, 2021

cmsbuild added geometry-approved and removed geometry-pending labels Jul 27, 2021

cmsbuild added db-approved fully-signed and removed db-pending pending-signatures labels Jul 27, 2021

cmsbuild mentioned this pull request Jul 27, 2021

Run 2, Run3, and Phase2 tracker: adding T27 (geometry with bricked pixels) and support for storing bricked pixel information #34120

Merged

cmsbuild added the urgent label Jul 27, 2021

ianna suggested changes Jul 27, 2021

View reviewed changes

cmsbuild added orp-approved and removed orp-pending labels Jul 27, 2021

cmsbuild merged commit 532acfe into cms-sw:master Jul 27, 2021

cvuosalo mentioned this pull request Jul 27, 2021

[High Priority][DD4hep] Improve design of DDCMS handling of assemblies and parsing context #34367

Closed

This was referenced Jul 27, 2021

Enable printout in ReusableObjectHolder test #34651

Merged

Fix crash on edit of Jet collection after view destruction #34655

Merged

[DD4hep] Tool for creation of DD4hep geometry DB payloads and for migration validation #34111

[DD4hep] Tool for creation of DD4hep geometry DB payloads and for migration validation #34111

Conversation

cvuosalo commented Jun 14, 2021 • edited

cmsbuild commented Jun 14, 2021

cmsbuild commented Jun 14, 2021

cmsbuild commented Jun 16, 2021

cmsbuild commented Jun 16, 2021

ianna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvuosalo commented Jun 16, 2021

ianna commented Jun 17, 2021

cmsbuild commented Jun 17, 2021

cmsbuild commented Jun 17, 2021

ianna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvuosalo commented Jul 25, 2021

cvuosalo commented Jul 25, 2021

cvuosalo commented Jul 26, 2021

cvuosalo commented Jul 26, 2021

cmsbuild commented Jul 27, 2021

Comparison Summary

civanch commented Jul 27, 2021

ggovi commented Jul 27, 2021

cmsbuild commented Jul 27, 2021

civanch commented Jul 27, 2021

ianna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvuosalo Jul 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qliphy commented Jul 27, 2021

cvuosalo commented Jun 14, 2021 •

edited

cvuosalo Jul 27, 2021 •

edited