Mini-PR: Minor style cleaning #32241

ghugo83 · 2020-11-23T10:21:47Z

This is a minor PR to improve the style of #32218, where I had not yet taken the time to clean.

Add comments and use of std::find_if in SimG4Core/Geometry/src/DD4hep_DDG4Builder.cc.
It is not needed anymore to close geometry in DetectorDescription/DDCMS/plugins/dd4hep/DDDefinitions2Objects.cc.

@ianna @civanch @cvuosalo

…/dd4hep/DDDefinitions2Objects.cc, as not needed to close geometry here. Suggestion from Ianna.

cmsbuild · 2020-11-23T10:32:25Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32241/19996

This PR adds an extra 16KB to repository

cmsbuild · 2020-11-23T10:32:51Z

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

It involves the following packages:

DetectorDescription/DDCMS
SimG4Core/Geometry

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

civanch · 2020-11-23T10:34:56Z

SimG4Core/Geometry/src/DD4hep_DDG4Builder.cc


-    edm::LogVerbatim("SimG4CoreApplication")
-        << " DDG4SensitiveConverter: Sensitive " << fff << " Class Name " << sClassName << " ROU Name " << sROUName;
+      edm::LogVerbatim("SimG4CoreApplication")


@ghugo83 , it may be too much to add the same printout as in line 67 ?

@civanch This is the printout of the reflected sensors when they exist, while the printout at line 67 is the printout of the non-reflected sensors.

civanch · 2020-11-23T14:39:37Z

please test

cmsbuild · 2020-11-23T14:39:58Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-11-23-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/10954/console Started: 2020/11/23 15:42

ianna

We ought to test if leaving geometry open is safe. Then ask for an option from DD4hep to keep it open. This will save us time on voxelization that we do not use :-)

ianna · 2020-11-23T14:47:02Z

DetectorDescription/DDCMS/plugins/dd4hep/DDDefinitions2Objects.cc

@@ -2243,9 +2243,6 @@ static long load_dddefinition(Detector& det, xml_h element) {
      if (mfv1.isValid())
        wv.placeVolume(mfv1, 1);

-      // Can not deal with reflections without closed geometry
-      det.manager().CloseGeometry();


@ghugo83 - there is no harm in closing geometry earlier. DD4hep will close it any way. At least we'd know when we forbid any further modifications to geometry.

@ianna It looks like we need to close the geometry.
When I revert this line, the tests pass.

ianna · 2020-11-23T14:55:09Z

+1

cmsbuild · 2020-11-23T16:12:09Z

-1

Tested at: d522cd9

CMSSW: CMSSW_11_2_X_2020-11-23-1100
SCRAM_ARCH: slc7_amd64_gcc820
You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e10e61/10954/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests RelVals

Unit Tests:

I found errors in the following unit tests:

---> test testDD4hepDDSolid had ERRORS

RelVals:

When I ran the RelVals I found an error in the following workflows:
11642.911 step2

runTheMatrix-results/11642.911_ZMM_13+2021_DD4hep+ZMM_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA/step2_ZMM_13+2021_DD4hep+ZMM_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA.log

11624.911 step2

runTheMatrix-results/11624.911_TTbar_13+2021_DD4hep+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA/step2_TTbar_13+2021_DD4hep+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA.log

cmsbuild · 2020-11-23T16:12:11Z

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

cmsbuild · 2020-11-23T17:18:55Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32241/20002

This PR adds an extra 16KB to repository

cmsbuild · 2020-11-23T17:19:16Z

Pull request #32241 was updated. @cmsbuild, @civanch, @mdhildreth can you please check and sign again.

ghugo83 · 2020-11-23T17:41:58Z

Tests should pass now

civanch · 2020-11-23T17:44:08Z

please test

cmsbuild · 2020-11-23T17:44:33Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-11-23-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/10959/console Started: 2020/11/23 18:45

cmsbuild · 2020-11-23T19:23:28Z

+1
Tested at: 6cbdb30
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e10e61/10959/summary.html
CMSSW: CMSSW_11_2_X_2020-11-23-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-11-23T19:23:30Z

Comparison job queued.

cmsbuild · 2020-11-23T20:47:19Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e10e61/10959/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 3 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2961011
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2960982
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 158 log files, 34 edm output root files, 37 DQM output files

civanch · 2020-11-23T21:30:40Z

+1

cmsbuild · 2020-11-23T21:31:08Z

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 · 2020-11-24T00:29:55Z

+1

ghugo83 added 3 commits November 23, 2020 10:58

Cleaning SimG4Core/Geometry/src/DD4hep_DDG4Builder.cc

dd74019

scram b code-format

df37883

Cleaning reminders of TGeo trick in DetectorDescription/DDCMS/plugins…

d522cd9

…/dd4hep/DDDefinitions2Objects.cc, as not needed to close geometry here. Suggestion from Ianna.

cmsbuild added this to the CMSSW_11_2_X milestone Nov 23, 2020

cmsbuild added code-checks-pending comparison-pending geometry-pending orp-pending pending-signatures simulation-pending tests-pending labels Nov 23, 2020

This was referenced Nov 23, 2020

Fix ECAL Endcap quadrants issue with DD4hep reflected volumes + Also fix ECAL Barrel and CSCs reflected active detectors #32218

Merged

Remove use of TGeo to convert reflections #32112

Merged

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 23, 2020

civanch reviewed Nov 23, 2020

View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Nov 23, 2020

ianna approved these changes Nov 23, 2020

View reviewed changes

cmsbuild added geometry-approved and removed geometry-pending labels Nov 23, 2020

cmsbuild removed comparison-pending tests-started labels Nov 23, 2020

cmsbuild added code-checks-pending comparison-pending tests-pending labels Nov 23, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 23, 2020

cmsbuild added tests-started and removed tests-pending labels Nov 23, 2020

cmsbuild added tests-approved and removed tests-started labels Nov 23, 2020

cmsbuild added comparison-available and removed comparison-pending labels Nov 23, 2020

cmsbuild added fully-signed simulation-approved and removed pending-signatures simulation-pending labels Nov 23, 2020

cmsbuild added orp-approved and removed orp-pending labels Nov 24, 2020

cmsbuild merged commit 044c077 into cms-sw:master Nov 24, 2020

cmsbuild mentioned this pull request Nov 24, 2020

ECAL SC parameters to EventSetup #32066

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mini-PR: Minor style cleaning #32241

Mini-PR: Minor style cleaning #32241

ghugo83 commented Nov 23, 2020 •

edited

cmsbuild commented Nov 23, 2020

cmsbuild commented Nov 23, 2020

civanch Nov 23, 2020

ghugo83 Nov 23, 2020

civanch commented Nov 23, 2020

cmsbuild commented Nov 23, 2020 •

edited

ianna left a comment

ianna Nov 23, 2020

ghugo83 Nov 23, 2020

ianna commented Nov 23, 2020

cmsbuild commented Nov 23, 2020

cmsbuild commented Nov 23, 2020

cmsbuild commented Nov 23, 2020

cmsbuild commented Nov 23, 2020

ghugo83 commented Nov 23, 2020

civanch commented Nov 23, 2020

cmsbuild commented Nov 23, 2020 •

edited

cmsbuild commented Nov 23, 2020

cmsbuild commented Nov 23, 2020

cmsbuild commented Nov 23, 2020

civanch commented Nov 23, 2020

cmsbuild commented Nov 23, 2020

qliphy commented Nov 24, 2020

Mini-PR: Minor style cleaning #32241

Mini-PR: Minor style cleaning #32241

Conversation

ghugo83 commented Nov 23, 2020 • edited

cmsbuild commented Nov 23, 2020

cmsbuild commented Nov 23, 2020

civanch Nov 23, 2020

Choose a reason for hiding this comment

ghugo83 Nov 23, 2020

Choose a reason for hiding this comment

civanch commented Nov 23, 2020

cmsbuild commented Nov 23, 2020 • edited

ianna left a comment

Choose a reason for hiding this comment

ianna Nov 23, 2020

Choose a reason for hiding this comment

ghugo83 Nov 23, 2020

Choose a reason for hiding this comment

ianna commented Nov 23, 2020

cmsbuild commented Nov 23, 2020

cmsbuild commented Nov 23, 2020

cmsbuild commented Nov 23, 2020

cmsbuild commented Nov 23, 2020

ghugo83 commented Nov 23, 2020

civanch commented Nov 23, 2020

cmsbuild commented Nov 23, 2020 • edited

cmsbuild commented Nov 23, 2020

cmsbuild commented Nov 23, 2020

cmsbuild commented Nov 23, 2020

civanch commented Nov 23, 2020

cmsbuild commented Nov 23, 2020

qliphy commented Nov 24, 2020

ghugo83 commented Nov 23, 2020 •

edited

cmsbuild commented Nov 23, 2020 •

edited

cmsbuild commented Nov 23, 2020 •

edited