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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from Dec 25, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
601df7b
Added DD4hep Units For CSCBuilder
slomeo Dec 17, 2020
0404b51
fixed some requests of Carl V.
slomeo Dec 18, 2020
1348192
put wg.wireSpacing = mm
slomeo Dec 20, 2020
59cbf2e
fixed Carl V. requests (now DDD and DD4HEP have the "same code" to ge…
slomeo Dec 22, 2020
25eb699
fixed LogVerbatim printout
slomeo Dec 22, 2020
8d3ef40
enable debug message in dump scripts
slomeo Dec 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
78 changes: 61 additions & 17 deletions Geometry/CSCGeometryBuilder/src/CSCGeometryParsFromDD.cc
View file
Open in desktop
Expand Up @@ -36,6 +36,8 @@ CSCGeometryParsFromDD::CSCGeometryParsFromDD() : myName("CSCGeometryParsFromDD")

CSCGeometryParsFromDD::~CSCGeometryParsFromDD() {}

//ddd

bool CSCGeometryParsFromDD::build(const DDCompactView* cview,
const MuonGeometryConstants& muonConstants,
RecoIdealGeometry& rig,
Expand Down Expand Up @@ -65,6 +67,9 @@ bool CSCGeometryParsFromDD::build(const DDCompactView* cview,
std::vector<double> gtran(3);
std::vector<double> grmat(9);
std::vector<double> trm(9);

edm::LogVerbatim("CSCGeometryParsFromDD") << "(0) CSCGeometryParsFromDD - DDD ";

while (doSubDets) {
spec = fv.specifics();
spit = spec.begin();
Expand All @@ -91,6 +96,10 @@ bool CSCGeometryParsFromDD::build(const DDCompactView* cview,
int jchamber = detid.chamber();
int jlayer = detid.layer();

edm::LogVerbatim("CSCGeometryParsFromDD")
<< "(1) detId: " << id << " jendcap: " << jendcap << " jstation: " << jstation << " jring: " << jring
<< " jchamber: " << jchamber << " jlayer: " << jlayer;

// Package up the wire group info as it's decoded
CSCWireGroupPackage wg;
uparvals.clear();
Expand Down Expand Up @@ -137,16 +146,21 @@ bool CSCGeometryParsFromDD::build(const DDCompactView* cview,
}
} else if (it->second.name() == "WireSpacing") {
wg.wireSpacing = it->second.doubles()[0];
edm::LogVerbatim("CSCGeometryParsFromDD") << "(2) wireSpacing: " << wg.wireSpacing;
} else if (it->second.name() == "AlignmentPinToFirstWire") {
wg.alignmentPinToFirstWire = it->second.doubles()[0];
edm::LogVerbatim("CSCGeometryParsFromDD") << "(3) alignmentPinToFirstWire: " << wg.alignmentPinToFirstWire;
} else if (it->second.name() == "TotNumWireGroups") {
wg.numberOfGroups = int(it->second.doubles()[0]);
} else if (it->second.name() == "LengthOfFirstWire") {
wg.narrowWidthOfWirePlane = it->second.doubles()[0];
edm::LogVerbatim("CSCGeometryParsFromDD") << "(4) narrowWidthOfWirePlane: " << wg.narrowWidthOfWirePlane;
} else if (it->second.name() == "LengthOfLastWire") {
wg.wideWidthOfWirePlane = it->second.doubles()[0];
edm::LogVerbatim("CSCGeometryParsFromDD") << "(5) wideWidthOfWirePlane: " << wg.wideWidthOfWirePlane;
} else if (it->second.name() == "RadialExtentOfWirePlane") {
wg.lengthOfWirePlane = it->second.doubles()[0];
edm::LogVerbatim("CSCGeometryParsFromDD") << "(6) lengthOfWirePlane: " << wg.lengthOfWirePlane;
}
}
}
Expand Down Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvuosalo : done

fpar.emplace_back((dpar[8] / cm));
Expand All @@ -214,6 +230,9 @@ bool CSCGeometryParsFromDD::build(const DDCompactView* cview,

LogTrace(myName) << myName << ": gtran[0]=" << gtran[0] << ", gtran[1]=" << gtran[1] << ", gtran[2]=" << gtran[2];

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

LogTrace(myName) << myName << ": fill grmat...";

fv.rotation().GetComponents(trm.begin(), trm.end());
Expand Down Expand Up @@ -369,6 +388,9 @@ bool CSCGeometryParsFromDD::build(const cms::DDCompactView* cview,
std::vector<double> gtran(3);
std::vector<double> grmat(9);
std::vector<double> trm(9);

edm::LogVerbatim("CSCGeometryParsFromDD") << "(0) CSCGeometryParsFromDD - DD4HEP ";

while (fv.firstChild()) {
MuonGeometryNumbering mbn(muonConstants);
CSCNumberingScheme cscnum(muonConstants);
Expand All @@ -381,6 +403,10 @@ bool CSCGeometryParsFromDD::build(const cms::DDCompactView* cview,
int jchamber = detid.chamber();
int jlayer = detid.layer();

edm::LogVerbatim("CSCGeometryParsFromDD")
<< "(1) detId: " << id << " jendcap: " << jendcap << " jstation: " << jstation << " jring: " << jring
<< " jchamber: " << jchamber << " jlayer: " << jlayer;

// Package up the wire group info as it's decoded
CSCWireGroupPackage wg;
uparvals.clear();
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@slomeo slomeo Dec 18, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

@slomeo slomeo Dec 18, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

edm::LogVerbatim("CSCGeometryParsFromDD") << "(2) wireSpacing: " << wg.wireSpacing / dd4hep::cm;

auto alignmentpintofirstwire = fv.get<double>("AlignmentPinToFirstWire");
wg.alignmentPinToFirstWire = static_cast<double>(alignmentpintofirstwire);
wg.alignmentPinToFirstWire = static_cast<double>(alignmentpintofirstwire / dd4hep::cm);
edm::LogVerbatim("CSCGeometryParsFromDD")
<< "(3) alignmentPinToFirstWire: " << wg.alignmentPinToFirstWire / dd4hep::cm;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't notice this before. wg.alignmentPinToFirstWire has already been converted to cm. It doesn't need a second conversion to cm. Same applies to the other debug print statements below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvuosalo : I added "/dd4hep::cm" in order to put the units in the code for a future fix even if alignmentPinToFirstWire is already in cm. Adding "/dd4hep::cm" do not change anything if the value of the parameter is already in "cm"


auto totnumwiregroups = fv.get<double>("TotNumWireGroups");
wg.numberOfGroups = static_cast<int>(totnumwiregroups);

auto lengthoffirstwire = fv.get<double>("LengthOfFirstWire");
wg.narrowWidthOfWirePlane = static_cast<double>(lengthoffirstwire);
wg.narrowWidthOfWirePlane = static_cast<double>(lengthoffirstwire / dd4hep::cm);
edm::LogVerbatim("CSCGeometryParsFromDD")
<< "(4) narrowWidthOfWirePlane: " << wg.narrowWidthOfWirePlane / dd4hep::cm;

auto lengthoflastwire = fv.get<double>("LengthOfLastWire");
wg.wideWidthOfWirePlane = static_cast<double>(lengthoflastwire);
wg.wideWidthOfWirePlane = static_cast<double>(lengthoflastwire / dd4hep::cm);
edm::LogVerbatim("CSCGeometryParsFromDD") << "(5) wideWidthOfWirePlane: " << wg.wideWidthOfWirePlane / dd4hep::cm;

auto radialextentofwireplane = fv.get<double>("RadialExtentOfWirePlane");
wg.lengthOfWirePlane = static_cast<double>(radialextentofwireplane);
wg.lengthOfWirePlane = static_cast<double>(radialextentofwireplane / dd4hep::cm);
edm::LogVerbatim("CSCGeometryParsFromDD") << "(6) lengthOfWirePlane: " << wg.lengthOfWirePlane / dd4hep::cm;

uparvals.emplace_back((wg.wireSpacing) * 10.0);
uparvals.emplace_back(wg.wireSpacing / dd4hep::mm);
Copy link
Contributor

@cvuosalo cvuosalo Dec 18, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to be very careful here. I don't think any values that go into the uparvals should be converted.
I'm also confused about wireSpacing. In the old DD code, wireSpacing is not changed, so why is it converted here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvuosalo : I deleted "/dd4hep::mm"inside uparvals and I set wireSpacing in mm

uparvals.emplace_back(wg.alignmentPinToFirstWire);
uparvals.emplace_back(wg.numberOfGroups);
uparvals.emplace_back(wg.narrowWidthOfWirePlane);
Expand Down Expand Up @@ -477,25 +510,36 @@ bool CSCGeometryParsFromDD::build(const cms::DDCompactView* cview,
std::transform(
dpar.begin(), dpar.end(), dpar.begin(), [](double i) -> double { return cms_rounding::roundIfNear0(i); });

fpar.emplace_back((dpar[1]));
fpar.emplace_back((dpar[2]));
fpar.emplace_back((dpar[3]));
fpar.emplace_back((dpar[4]));
fpar.emplace_back((dpar[1] / dd4hep::cm));
fpar.emplace_back((dpar[2] / dd4hep::cm));
fpar.emplace_back((dpar[3] / dd4hep::cm));
fpar.emplace_back((dpar[4] / dd4hep::cm));
edm::LogVerbatim("CSCGeometryParsFromDD")
<< "(7) - Subtraction - dpar[1] (ddd dpar[4]): " << dpar[1] / dd4hep::cm
<< " (ddd dpar[8]): " << dpar[2] / dd4hep::cm << " dpar[3] (as ddd): " << dpar[3] / dd4hep::cm
<< " dpar[4] (ddd dpar[0]): " << dpar[4] / dd4hep::cm;
} else {
dpar = fv.parameters();

std::transform(
dpar.begin(), dpar.end(), dpar.begin(), [](double i) -> double { return cms_rounding::roundIfNear0(i); });

fpar.emplace_back((dpar[0]));
fpar.emplace_back((dpar[1]));
fpar.emplace_back((dpar[2]));
fpar.emplace_back((dpar[3]));
fpar.emplace_back((dpar[0] / dd4hep::cm));
fpar.emplace_back((dpar[1] / dd4hep::cm));
fpar.emplace_back((dpar[2] / dd4hep::cm));
fpar.emplace_back((dpar[3] / dd4hep::cm));
edm::LogVerbatim("CSCGeometryParsFromDD")
<< "(7)Bis - Else - dpar[0]: " << dpar[0] / dd4hep::cm << " dpar[1]: " << dpar[1] / dd4hep::cm
<< " dpar[2]: " << dpar[2] / dd4hep::cm << " dpar[3]: " << dpar[3] / dd4hep::cm;
}

gtran[0] = static_cast<float>(fv.translation().X());
gtran[1] = static_cast<float>(fv.translation().Y());
gtran[2] = static_cast<float>(fv.translation().Z());
gtran[0] = static_cast<float>(fv.translation().X() / dd4hep::cm);
gtran[1] = static_cast<float>(fv.translation().Y() / dd4hep::cm);
gtran[2] = static_cast<float>(fv.translation().Z() / dd4hep::cm);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ";
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvuosalo : done


std::transform(
gtran.begin(), gtran.end(), gtran.begin(), [](double i) -> double { return cms_rounding::roundIfNear0(i); });
Expand Down
10 changes: 8 additions & 2 deletions Geometry/CSCGeometryBuilder/test/python/validateCSCGeometryDD4hep_cfg.py
View file
Open in desktop
Expand Up @@ -9,12 +9,18 @@
input = cms.untracked.int32(1)
)


process.load('Configuration.StandardSequences.DD4hep_GeometrySim_cff')
process.load("FWCore.MessageLogger.MessageLogger_cfi")
process.load("Geometry.MuonNumbering.muonGeometryConstants_cff")
process.load("Geometry.CSCGeometryBuilder.cscGeometry_cfi")

process.MessageLogger = cms.Service("MessageLogger",
destinations = cms.untracked.vstring('myLog'),
myLog = cms.untracked.PSet(
threshold = cms.untracked.string('INFO'),
)
)

process.CSCGeometryESModule.applyAlignment = False

#
Expand All @@ -25,7 +31,7 @@
#
process.valid = cms.EDAnalyzer("CSCGeometryValidate",
infileName = cms.untracked.string('cmsRecoGeom-2021.root'),
outfileName = cms.untracked.string('validateCSCGeometry.root'),
outfileName = cms.untracked.string('validateCSCGeometryDD4HEP.root'),
tolerance = cms.untracked.int32(7)
)

Expand Down
9 changes: 8 additions & 1 deletion Geometry/CSCGeometryBuilder/test/python/validateCSCGeometryDDD_cfg.py
View file
Open in desktop
Expand Up @@ -14,6 +14,13 @@
process.load("Geometry.MuonNumbering.muonNumberingInitialization_cfi")
process.load("Geometry.CSCGeometryBuilder.cscGeometry_cfi")

process.MessageLogger = cms.Service("MessageLogger",
destinations = cms.untracked.vstring('myLog'),
myLog = cms.untracked.PSet(
threshold = cms.untracked.string('INFO'),
)
)

process.CSCGeometryESModule.applyAlignment = False

#
Expand All @@ -24,7 +31,7 @@
#
process.valid = cms.EDAnalyzer("CSCGeometryValidate",
infileName = cms.untracked.string('cmsRecoGeom-2021.root'),
outfileName = cms.untracked.string('validateCSCGeometry2.root'),
outfileName = cms.untracked.string('validateCSCGeometryDDD.root'),
tolerance = cms.untracked.int32(7)
)

Expand Down