Make units dd4hep-dependent in PPS #32538

ghugo83 · 2020-12-18T10:01:28Z

Make units of values from XMLs dd4hep-dependent.
This will allow the values to change automatically, when DD4hep changes its reference unit.

@ianna @cvuosalo @civanch

…values to change automatically when DD4hep changes its reference unit.

cmsbuild · 2020-12-18T10:09:38Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32538/20490

This PR adds an extra 16KB to repository

cmsbuild · 2020-12-18T10:10:02Z

A new Pull Request was created by @ghugo83 for master.

It involves the following packages:

Geometry/VeryForwardGeometryBuilder

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild can you please review it and eventually sign? Thanks.
@forthommel, @jan-kaspar, @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-18T10:41:42Z

please test

cmsbuild · 2020-12-18T15:33:27Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16034c/11790/summary.html
CMSSW: CMSSW_11_3_X_2020-12-17-2300/slc7_amd64_gcc900

DAS Queries failed

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 36
DQMHistoTests: Total histograms compared: 2716967
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2716944
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 35 files compared)
Checked 153 log files, 37 edm output root files, 36 DQM output files

cvuosalo · 2020-12-18T17:50:43Z

Geometry/VeryForwardGeometryBuilder/src/DetGeomDesc.cc

@@ -152,9 +152,7 @@ DiamondDimensions DetGeomDesc::computeDiamondDimensions(const bool isABox,
      boxShapeParameters = {params.at(0), params.at(1), params.at(2)};
    } else {
      // convert cm (DD4hep) to mm (legacy expected by PPS reco software)


Comments on lines 50, 57, and 154 need to be updated. DD4hep units are not necessarily cm anymore.

// Convert DD4hep units to mm (legacy expected by PPS reco software)

@cvuosalo Same, sorry will quickly address the comment tonight when I am back to a computer

cvuosalo · 2020-12-18T18:01:06Z

Geometry/VeryForwardGeometryBuilder/src/DetGeomDesc.cc

@@ -47,14 +47,14 @@ DetGeomDesc::DetGeomDesc(const cms::DDFilteredView& fv, const bool isRun2)
    : m_name(computeNameWithNoNamespace(fv.name())),
      m_copy(fv.copyNum()),
      m_isDD4hep(true),
-      m_trans(geant_units::operators::convertCmToMm(fv.translation())),  // converted from cm (DD4hep) to mm
+      m_trans(fv.translation() / dd4hep::mm),  // converted from cm (DD4hep) to mm
      m_rot(fv.rotation()),
      m_params(computeParameters(fv)),  // default unit from DD4hep (cm)


I would convert m_params to mm right here (actually in computeParameters). Then all members of DetGeomDesc would be in Geant4 units. computeDiamondDimensions could then be simplified because the input units would be always mm.

When old DD is removed, that case will disappear anyway.
In the meantime, I would not make old DD case depend on the DD4hep units, while the DD4hep case needs the /dd4hep::mm to have the certainty of getting mm regardless of the DD4hep internals.

@ghugo83 I am saying the opposite. Make the interface consistent. For DetGeomDesc members and methods, all units should be mm. Right now, the params method (which is never used, as far as I can tell) and m_params store DD4hep units, while all the other members and methods use mm.
If m_params in the DD4hep version were converted to mm at initialization, then computeDiamondDimensions would expect mm only, and it could be simplified by the removal of the if.

@cvuosalo Ha yes, I looked at it more properly, sorry I see what you mean now.
params() is used (from legacy) in CondTools/Geometry/plugins/PGeometricDetBuilder.cc to store PGeometricDet info to DB.
It is true that it would be easier to just convert all shape parameters to mm in a computeParameters function. The computeDiamondDimensions would just be a particular case of this, in the event that the shape is a box.

It is actually the version which I had at first.
But the thing is that it was implying a specific treatment for each possible shape, because we want to convert only the parameters which are lengths, and not all the parameters of the shape.
This was making the computeParameters() function quite heavy.
And in practice, as you noticed, only the box shape parameters are used anyway in the PPS legacy code.

Hence, after discussion with the PPS team, it was decided that the shape parameters are only treated in the case of a box, with the computeDiamondDimensions and getDiamondDimensions functions.
The params() function instead, which provides info to DB, is kept untouched, and just provides the shape parameters in their native format (units, but also orders of parameters).

Shall we want to simplify this, we could just remove params() function - but that would imply removing (useless) info stored in DB in legacy, hence changing the CondFormats objects, hence PPS team preferred keeping that params() function as-is, and just having a special treatment for boxes.

Does this make more sense? :)

ghugo83 · 2020-12-21T22:03:15Z

Pushed the comments :)

cmsbuild · 2021-01-04T17:08:30Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32538/20632

This PR adds an extra 20KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32538/20632/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32538/20632/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2021-01-04T17:27:40Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32538/20633

This PR adds an extra 20KB to repository

cmsbuild · 2021-01-04T17:28:04Z

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

civanch · 2021-01-04T18:23:42Z

please test

cmsbuild · 2021-01-04T21:12:11Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16034c/11950/summary.html
CMSSW: CMSSW_11_3_X_2021-01-04-1100/slc7_amd64_gcc900

Comparison Summary

Summary:

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

civanch · 2021-01-05T16:08:52Z

+1

Carl's comments are addressed

cmsbuild · 2021-01-05T16:09:14Z

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)

qliphy · 2021-01-06T00:05:01Z

+1

Make units of values from XMLs dd4hep-dependent. This will allow the …

c63883d

…values to change automatically when DD4hep changes its reference unit.

cmsbuild added this to the CMSSW_11_3_X milestone Dec 18, 2020

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

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

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

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

cvuosalo reviewed Dec 18, 2020

View reviewed changes

cmsbuild added code-checks-pending core-pending hlt-pending pdmv-pending reconstruction-pending simulation-pending tests-pending upgrade-pending and removed code-checks-approved tests-approved labels Dec 21, 2020

Address comments

dcbe32f

cmsbuild added code-checks-pending and removed code-checks-approved labels Jan 4, 2021

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jan 4, 2021

scram b code-format

4c4d7c4

cmsbuild added code-checks-pending and removed code-checks-rejected labels Jan 4, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 4, 2021

cmsbuild added tests-started and removed tests-pending labels Jan 4, 2021

cmsbuild added tests-approved and removed tests-started labels Jan 4, 2021

cmsbuild mentioned this pull request Jan 5, 2021

PPS: fix z sign in diamond geometry #32603

Merged

cmsbuild added fully-signed geometry-approved and removed geometry-pending pending-signatures labels Jan 5, 2021

cmsbuild added orp-approved and removed orp-pending labels Jan 6, 2021

cmsbuild merged commit 40a52b9 into cms-sw:master Jan 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make units dd4hep-dependent in PPS #32538

Make units dd4hep-dependent in PPS #32538

ghugo83 commented Dec 18, 2020 •

edited

cmsbuild commented Dec 18, 2020

cmsbuild commented Dec 18, 2020

civanch commented Dec 18, 2020

cmsbuild commented Dec 18, 2020

cvuosalo Dec 18, 2020

ghugo83 Dec 21, 2020

cvuosalo Dec 18, 2020

ghugo83 Dec 21, 2020

cvuosalo Dec 21, 2020

ghugo83 Jan 4, 2021 •

edited

ghugo83 commented Dec 21, 2020

cmsbuild commented Jan 4, 2021

cmsbuild commented Jan 4, 2021

cmsbuild commented Jan 4, 2021

civanch commented Jan 4, 2021

cmsbuild commented Jan 4, 2021

civanch commented Jan 5, 2021

cmsbuild commented Jan 5, 2021

qliphy commented Jan 6, 2021

Make units dd4hep-dependent in PPS #32538

Make units dd4hep-dependent in PPS #32538

Conversation

ghugo83 commented Dec 18, 2020 • edited

cmsbuild commented Dec 18, 2020

cmsbuild commented Dec 18, 2020

civanch commented Dec 18, 2020

cmsbuild commented Dec 18, 2020

Comparison Summary

cvuosalo Dec 18, 2020

Choose a reason for hiding this comment

ghugo83 Dec 21, 2020

Choose a reason for hiding this comment

cvuosalo Dec 18, 2020

Choose a reason for hiding this comment

ghugo83 Dec 21, 2020

Choose a reason for hiding this comment

cvuosalo Dec 21, 2020

Choose a reason for hiding this comment

ghugo83 Jan 4, 2021 • edited

Choose a reason for hiding this comment

ghugo83 commented Dec 21, 2020

cmsbuild commented Jan 4, 2021

cmsbuild commented Jan 4, 2021

cmsbuild commented Jan 4, 2021

civanch commented Jan 4, 2021

cmsbuild commented Jan 4, 2021

Comparison Summary

civanch commented Jan 5, 2021

cmsbuild commented Jan 5, 2021

qliphy commented Jan 6, 2021

ghugo83 commented Dec 18, 2020 •

edited

ghugo83 Jan 4, 2021 •

edited