Added "/dd4hep::cm" to the CSCGeometryParsFromDD Class #32526

slomeo · 2020-12-17T17:12:53Z

PR description:

Added "/ ddhep::cm" to the DD4hep part of the CScGeometryParsFromDD Class in order to simplify a possible future unit transition

PR validation:

validation by "cmsRun Geometry/CSCGeometryBuilder/test/python/dumpCSCGeometryDDD_cfg.py" and "cmsRun Geometry/CSCGeometryBuilder/test/python/dumpCSCGeometryDD4hep_cfg.py". Save the output of each dump script and look at these values:
wireSpacing = 0.25, y1 = -79.9354, narrow_width = 20.13, wide_width = 48.71, length = 150.5, wireAngle = 0.506145, theWireOffset = -69.9131
They have to be the same for DDD and DD4hep.
validation by "cmsRun Geometry/CSCGeometryBuilder/test/python/validateCSCGeometryDD4Hep_cfg.py" and "cmsRun Geometry/RPCGeometryBuilder/test/python/validateDTGeometryDDD_cfg.py"

See attached picture (only one histo for DD4hep)

validation by printouts (myLog.log), for both DD & DD4Hep, created by the validation scripts used above (for example please see, below, the parameters check for the Det Id 604017672)

//DDD

(0) CSCGeometryParsFromDD - DDD
(1) detId: 604017672 jendcap: 1 jstation: 1 jring: 1 jchamber: 1 jlayer: 0
(7) dpar[4]: 15.065 cm dpar[8]: 30.45 cm dpar[3]: 7.35 cm dpar[0]: 81 cm
(8) gtran[0]: 181.5 cm gtran[1]: -3.15941e-13 cm gtran[2]: 616.95 cm

//DD4Hep

(0) CSCGeometryParsFromDD - DD4HEP
(1) detId: 604017672 jendcap: 1 jstation: 1 jring: 1 jchamber: 1 jlayer: 0
(7) dpar[0]: 15.065 cm dpar[1]: 30.45 cm dpar[2]: 7.35 cm dpar[3]: 81 cm
(8) gtran[0]: 181.5 cm gtran[1]: -4.46384e-14 cm gtran[2]: 616.95 cm

validation by "runTheMatrix.py -l 25202.1" :

25202.1_TTbar_13+TTbar_13+DIGIUP15APVSimu_PU25+RECOUP15_PU25+HARVESTUP15_PU25 Step0-PASSED Step1-PASSED Step2-PASSED Step3-PASSED - time date Thu Dec 17 17:44:41 2020-date Thu Dec 17 17:22:02 2020; exit: 0 0 0 0 1 1 1 1 tests passed, 0 0 0 0 failed

if this PR is a backport please specify the original PR and why you need to backport that PR:

nothing special

cmsbuild · 2020-12-17T17:19:51Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32526/20477

This PR adds an extra 20KB to repository

cmsbuild · 2020-12-17T17:20:10Z

A new Pull Request was created by @slomeo (Sergio Lo Meo) for master.

It involves the following packages:

Geometry/CSCGeometryBuilder

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

civanch · 2020-12-17T20:13:44Z

please test

cvuosalo · 2020-12-17T21:10:08Z

Geometry/CSCGeometryBuilder/src/CSCGeometryParsFromDD.cc

@@ -200,6 +214,8 @@ bool CSCGeometryParsFromDD::build(const DDCompactView* cview,
    LogTrace(myName) << myName << ": fill fpar...";
    LogTrace(myName) << myName << ": dpars are... " << dpar[4] / cm << ", " << dpar[8] / cm << ", " << dpar[3] / cm
                     << ", " << dpar[0] / cm;
+    edm::LogVerbatim("CSCGeometryParsFromDD") << "(7) dpar[4]: " << dpar[4] / cm << " dpar[8]:  " << dpar[8] / cm
+                                              << " dpar[3]: " << dpar[3] / cm << " dpar[0]: " << dpar[0] / cm;

    fpar.emplace_back((dpar[4] / cm));


Please delete line 24:

#include "CLHEP/Units/GlobalSystemOfUnits.h"

(We are trying to remove this file from all of Geometry code.)
Then these lines need to be changed like:

fpar.emplace_back(convertMmToCm(dpar[4]));

@cvuosalo : done

cvuosalo · 2020-12-17T21:20:45Z

Geometry/CSCGeometryBuilder/src/CSCGeometryParsFromDD.cc

@@ -426,24 +452,31 @@ bool CSCGeometryParsFromDD::build(const cms::DDCompactView* cview,
      }

      auto wirespacing = fv.get<double>("WireSpacing");
-      wg.wireSpacing = static_cast<double>(wirespacing);
+      wg.wireSpacing = static_cast<double>(wirespacing / dd4hep::cm);


I suggest simplifying the treatment of wireSpacing. Convert it to mm at the start.

wg.wireSpacing = fv.get<double>("WireSpacing") / dd4hep::mm;

In the debug output, give "mm" as its units. And then in line 479, there is no conversion because it is already in mm.

@cvuosalo : wg.wireSpacing is read, by DD4HEP, in "cm" (see PR description within the DD4HEP part) so in line 479 it is neessary to put / dd4hep::mm.

This code goes through three steps with wire spacing:

auto wirespacing = fv.get<double>("WireSpacing"); // Get wire spacing in DD4hep units wg.wireSpacing = static_cast<double>(wirespacing / dd4hep::cm); // Convert wire spacing to cm uparvals.emplace_back(wg.wireSpacing / dd4hep::mm); // Store wire spacing in mm

I don't see any reason for the intermediate conversion to cm. Why is that needed? Don't convert it to cm. It could be converted to mm in the first step in line 456, and that would be the only conversion:

wg.wireSpacing = fv.get<double>("WireSpacing") / dd4hep::mm;

@cvuosalo : before adding "/ dd4hep::cm" in the line "wg.wireSpacing = static_cast(wirespacing / dd4hep::cm);" I have checked the printouts written inside the myLog.lo file and I saw that for the DD4HEP part wirespacing was written in "cm" not in "mm" (i.e without "/dd4hep::cm"). All parameters inside the DD4HEP part are in "cm". So I added "/dd4hep::cm" in order to put the units in the code for a future fix. I noted that adding "/dd4hep::cm" in all the parameters that are already "cm" does not change the value. The only part of the DD4HEP code written in "mm" was "uparvals.emplace_back((wg.wireSpacing) * 10.0);" in line 446. This is why I wrote /dd4hep::cm and /dd4hep::mm in the code.

Yes, the code here is designed in an inconsistent way. alignmentPinToFirstWire, narrowWidthOfWirePlane, and others are stored in cm. However, wireSpacing is stored in mm. I am suggesting to make a change so wireSpacing is changed to mm at the first step, which will require changing the debug print-out statements so they say that wireSpacing is in mm.
The reason we are changing the DD4hep units is to reduce the number of multiplications. Since wireSpacing is stored in mm, it never needs to be in cm, so let's eliminate the step that converts wireSpacing to cm.

cvuosalo · 2020-12-17T21:22:31Z

Geometry/CSCGeometryBuilder/src/CSCGeometryParsFromDD.cc

+
+    edm::LogVerbatim("CSCGeometryParsFromDD")
+        << "(8) gtran[0]: " << gtran[0] / dd4hep::cm << " gtran[1]: " << gtran[1] / dd4hep::cm
+        << " gtran[2]: " << gtran[2] / dd4hep::cm;


I noticed lines 557-562 contain a useless loop. Those lines can be changed to:

if (wg.numberOfGroups == 0) { LogTrace(myName) << myName << " wg.numberOfGroups == 0 "; }

@cvuosalo : done

cmsbuild · 2020-12-17T23:06:15Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-980269/11776/summary.html
CMSSW: CMSSW_11_3_X_2020-12-17-1100/slc7_amd64_gcc900

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 7 differences found in the comparisons
DQMHistoTests: Total files compared: 36
DQMHistoTests: Total histograms compared: 2747287
DQMHistoTests: Total failures: 15
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2747249
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 35 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 153 log files, 37 edm output root files, 36 DQM output files

cmsbuild · 2020-12-18T13:50:29Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32526/20497

This PR adds an extra 20KB to repository

cmsbuild · 2020-12-18T13:50:49Z

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

civanch · 2020-12-18T15:19:11Z

please test

cmsbuild · 2020-12-24T08:09:53Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32526/20572

This PR adds an extra 20KB to repository

cmsbuild · 2020-12-24T08:10:18Z

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

civanch · 2020-12-24T09:13:43Z

please test

cmsbuild · 2020-12-24T12:05:59Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-980269/11865/summary.html
CMSSW: CMSSW_11_3_X_2020-12-23-2300/slc7_amd64_gcc900

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-980269/1325.81_TTbar_13_106Xv1NanoAODINPUT+TTbar_13_106Xv1NanoAODINPUT+NANOEDMMC2017_106XMiniAODv1+HARVESTNANOAODMC2017_106XMiniAODv1
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-980269/136.8523_RunJetHT2018C_nanoULremini+RunJetHT2018C_nanoULremini+NANOEDM2018_106Xv2+HARVESTNANOAOD2018_106Xv2

Summary:

No significant changes to the logs found
Reco comparison results: 605 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2716967
DQMHistoTests: Total failures: 1853
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2715091
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 36 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 156 log files, 37 edm output root files, 37 DQM output files

cvuosalo · 2020-12-24T14:12:10Z

@slomeo Sorry, I left out a step: git cms-addpkg Geometry/CSCGeometry. Then compile with debugging.
The test should output lines like:

 wireSpacing = 0.25, y1 = -79.9354, narrow_width = 20.13, wide_width = 48.71, length = 150.5, wireAngle = 0.506145, theWireOffset = -69.9131

The values in these lines should be identical between old DD and DD4hep.

slomeo · 2020-12-24T16:16:52Z

@slomeo Sorry, I left out a step: git cms-addpkg Geometry/CSCGeometry. Then compile with debugging.

@cvuosalo : I already have the Geometry/CSCGeometry directory in my IB. I have compiled and I followed your instructions (i.e scram b -j8 USER_CXXFLAGS="-DEDM_ML_DEBUG). The results are in /afs/cern.ch/user/s/slomeo/public/forCarl

cvuosalo · 2020-12-24T17:43:19Z

@slomeo Yes, I checked the results in the forCarl directory. They do not show the narrow_width message. I will explain more in my next comment.

cvuosalo · 2020-12-24T18:05:16Z

I have confirmed that the latest version of this PR is correct and that the current master has a bug that is fixed by this PR. For the record, here are the instructions and results.
Check master:

cmsrel CMSSW_11_3_X_2020-12-24-1100
git cms-addpkg Geometry/CSCGeometry
git cms-addpkg Geometry/CSCGeometryBuilder
env USER_CXXFLAGS="-g -DEDM_ML_DEBUG -DEDM_ML_LOGDEBUG" scram b -v -j 8
Edit Geometry/CSCGeometryBuilder/test/python/dumpCSCGeometryDDD_cfg.py and Geometry/CSCGeometryBuilder/test/python/dumpCSCGeometryDD4hep_cfg.py to show all DEBUG messages (see a previous comment for details).
cmsRun the these two dump-CSC-geometry scripts and save their output.
In the DDD output there should be a line:
wireSpacing = 0.25, y1 = -79.9354, narrow_width = 20.13, wide_width = 48.71, length = 150.5, wireAngle = 0.506145, theWireOffset = -69.9131
while in the DD4hep output the line is:
wireSpacing = 0.25, y1 = -80.8935, narrow_width = 2.013, wide_width = 4.871, length = 15.05, wireAngle = 0.506145, theWireOffset = -70.7511
Note the differences in the values. The DD4hep output is incorrect. The master has a bug (which is fixed by this PR).

Repeat these steps for this PR. Note that enabling DEBUG messages in the dump-CSC-geometry scripts isn't needed since this PR has already done that. With this PR, the key line of debug output matches between old DD and DD4hep:
wireSpacing = 0.25, y1 = -79.9354, narrow_width = 20.13, wide_width = 48.71, length = 150.5, wireAngle = 0.506145, theWireOffset = -69.9131
This PR is correctly handing the width and length values that are derived from spec pars in the XML files, and it fixes the bug in the master.

cvuosalo · 2020-12-24T18:27:03Z

+1

In the comparison test results for the DD4hep workflows, I see some improved results (histograms filled in that were empty in the reference) and also irrelevant statistical fluctuations. I think the comparison results show that this PR fixed a small bug and thus the results changed in a positive way.

cmsbuild · 2020-12-24T18:30:43Z

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)

slomeo · 2020-12-24T20:45:53Z

@cvuosalo thank you very much!

cvuosalo · 2020-12-24T20:54:35Z

@slomeo Could you please update the validation section of the PR description with the output of the final version of the validation test? You changed the PR after you wrote the PR description. Also, correct the name of the validation script (it's not Geometry/RPCGeometryBuilder/test/python/validateDTGeometryDDD_cfg.py).
Thanks.

slomeo · 2020-12-24T21:26:02Z

@slomeo Could you please update the validation section of the PR description

@cvuosalo : done. Happy Holidays.

qliphy · 2020-12-25T00:06:52Z

+1

Added DD4hep Units For CSCBuilder

601df7b

cmsbuild added this to the CMSSW_11_3_X milestone Dec 17, 2020

cmsbuild added code-checks-pending geometry-pending orp-pending pending-signatures tests-pending labels Dec 17, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 17, 2020

cmsbuild added tests-started and removed tests-pending labels Dec 17, 2020

cvuosalo reviewed Dec 17, 2020

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Dec 17, 2020

fixed some requests of Carl V.

0404b51

cmsbuild added code-checks-pending tests-pending and removed code-checks-approved tests-approved labels Dec 18, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 18, 2020

enable debug message in dump scripts

8d3ef40

cmsbuild added code-checks-pending and removed code-checks-approved labels Dec 24, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 24, 2020

cmsbuild added tests-started and removed tests-pending labels Dec 24, 2020

cmsbuild added tests-approved and removed tests-started labels Dec 24, 2020

cmsbuild added fully-signed geometry-approved and removed geometry-pending pending-signatures labels Dec 24, 2020

cmsbuild added orp-approved and removed orp-pending labels Dec 25, 2020

cmsbuild merged commit 8016b92 into cms-sw:master Dec 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added "/dd4hep::cm" to the CSCGeometryParsFromDD Class #32526

Added "/dd4hep::cm" to the CSCGeometryParsFromDD Class #32526

slomeo commented Dec 17, 2020 •

edited

cmsbuild commented Dec 17, 2020

cmsbuild commented Dec 17, 2020

civanch commented Dec 17, 2020

cvuosalo Dec 17, 2020

slomeo Dec 18, 2020

cvuosalo Dec 17, 2020

slomeo Dec 18, 2020 •

edited

cvuosalo Dec 18, 2020

slomeo Dec 18, 2020 •

edited

cvuosalo Dec 18, 2020

cvuosalo Dec 17, 2020

slomeo Dec 18, 2020

cmsbuild commented Dec 17, 2020

cmsbuild commented Dec 18, 2020

cmsbuild commented Dec 18, 2020

civanch commented Dec 18, 2020

cmsbuild commented Dec 24, 2020

cmsbuild commented Dec 24, 2020

civanch commented Dec 24, 2020

cmsbuild commented Dec 24, 2020

cvuosalo commented Dec 24, 2020

slomeo commented Dec 24, 2020

cvuosalo commented Dec 24, 2020

cvuosalo commented Dec 24, 2020

cvuosalo commented Dec 24, 2020

cmsbuild commented Dec 24, 2020

slomeo commented Dec 24, 2020

cvuosalo commented Dec 24, 2020

slomeo commented Dec 24, 2020

qliphy commented Dec 25, 2020

Added "/dd4hep::cm" to the CSCGeometryParsFromDD Class #32526

Added "/dd4hep::cm" to the CSCGeometryParsFromDD Class #32526

Conversation

slomeo commented Dec 17, 2020 • edited

cmsbuild commented Dec 17, 2020

cmsbuild commented Dec 17, 2020

civanch commented Dec 17, 2020

cvuosalo Dec 17, 2020

Choose a reason for hiding this comment

slomeo Dec 18, 2020

Choose a reason for hiding this comment

cvuosalo Dec 17, 2020

Choose a reason for hiding this comment

slomeo Dec 18, 2020 • edited

Choose a reason for hiding this comment

cvuosalo Dec 18, 2020

Choose a reason for hiding this comment

slomeo Dec 18, 2020 • edited

Choose a reason for hiding this comment

cvuosalo Dec 18, 2020

Choose a reason for hiding this comment

cvuosalo Dec 17, 2020

Choose a reason for hiding this comment

slomeo Dec 18, 2020

Choose a reason for hiding this comment

cmsbuild commented Dec 17, 2020

Comparison Summary

cmsbuild commented Dec 18, 2020

cmsbuild commented Dec 18, 2020

civanch commented Dec 18, 2020

cmsbuild commented Dec 24, 2020

cmsbuild commented Dec 24, 2020

civanch commented Dec 24, 2020

cmsbuild commented Dec 24, 2020

Comparison Summary

cvuosalo commented Dec 24, 2020

slomeo commented Dec 24, 2020

cvuosalo commented Dec 24, 2020

cvuosalo commented Dec 24, 2020

cvuosalo commented Dec 24, 2020

cmsbuild commented Dec 24, 2020

slomeo commented Dec 24, 2020

cvuosalo commented Dec 24, 2020

slomeo commented Dec 24, 2020

qliphy commented Dec 25, 2020

slomeo commented Dec 17, 2020 •

edited

slomeo Dec 18, 2020 •

edited

slomeo Dec 18, 2020 •

edited