[DD4hep] Filtered View is not a Shape #31603

ianna · 2020-09-28T11:21:39Z

PR description:

Shape type check is performed on a FilteredView current shape rather then on a View itself

Yet another small step towards code migration to the DD4hep repo.

@cvuosalo, @namapane - unfortunately, it's bundled with #31578

PR validation:

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

Before submitting your pull requests, make sure you followed this checklist:

verify that the PR is really intended for the chosen branch
verify that changes follow CMS Naming, Coding, And Style Rules
verify that the PR passes the basic test procedure suggested in the CMSSW PR instructions

…na with cms-merge-topic

cmsbuild · 2020-09-28T11:22:05Z

The code-checks are being triggered in jenkins.

ianna · 2020-09-28T11:22:15Z

please test

cmsbuild · 2020-09-28T11:29:03Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31603/18660

This PR adds an extra 44KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DetectorDescription/DDCMS/interface/DDShapes.h modified in PR(s): [DD4hep] Remove Shape Wrapper Classes #31581
- File DetectorDescription/DDCMS/src/DDShapes.cc modified in PR(s): [DD4hep] Remove Shape Wrapper Classes #31581
- File Geometry/VeryForwardGeometryBuilder/src/DetGeomDesc.cc modified in PR(s): [PPS] 2021 conditions for timing detectors geometry (resubmission after DD4hep) #31484
- File MagneticField/GeomBuilder/src/DD4hep_volumeHandle.cc modified in PR(s): [DD4hep] MagneticField Builder Shapes Use Cleanup - Use dd4hep::Solid Directly #31578

cmsbuild · 2020-09-28T11:29:29Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-09-28-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/9613/console Started: 2020/09/28 14:54

cmsbuild · 2020-09-28T11:29:30Z

A new Pull Request was created by @ianna (Ianna Osborne) for master.

It involves the following packages:

DetectorDescription/DDCMS
Geometry/EcalCommonData
Geometry/HGCalCommonData
Geometry/HcalCommonData
Geometry/HcalTestBeamData
Geometry/MTDCommonData
Geometry/RPCGeometryBuilder
Geometry/VeryForwardGeometryBuilder
MagneticField/GeomBuilder

@perrotta, @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88, @slava77, @jpata can you please review it and eventually sign? Thanks.
@namapane, @vargasa, @rchatter, @forthommel, @jan-kaspar, @argiro, @fabiocos, @slomeo 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

namapane · 2020-09-28T11:48:38Z

@ianna OK but now I don't know how to proceed. The plan was to replace #31578 with a different PR avoiding the replication of code into DD4hep_volumeHandle.cc. I don't know what to do at this point...

ianna · 2020-09-28T12:52:16Z

@ianna OK but now I don't know how to proceed. The plan was to replace #31578 with a different PR avoiding the replication of code into DD4hep_volumeHandle.cc. I don't know what to do at this point...

@namapane - when your PR is ready, I'll rebase this one.

cmsbuild · 2020-09-28T14:16:22Z

-1

Tested at: 05dee07

CMSSW: CMSSW_11_2_X_2020-09-28-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-5c9ce2/9613/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests

Unit Tests:

I found errors in the following unit tests:

---> test testDD4hepFilteredViewLevel had ERRORS
---> test testDD4hepFilteredViewGoTo had ERRORS

cmsbuild · 2020-09-28T14:16:24Z

Comparison job queued.

cmsbuild · 2020-09-28T15:44:10Z

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

Comparison Summary:

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

cmsbuild · 2020-09-28T17:36:29Z

+1
Tested at: 05dee07
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-36d0af/9627/summary.html
CMSSW: CMSSW_11_2_X_2020-09-28-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-09-28T17:36:35Z

Comparison job queued.

cmsbuild · 2020-09-28T19:01:38Z

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

Comparison Summary:

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

kpedro88 · 2020-10-01T16:55:15Z

+upgrade

jpata

I'm ready to approve it from the reco side, given the limited/minor reco changes in MagneticField/GeomBuilder. I just had a minor comment and a question - please see above.

jpata · 2020-10-02T09:07:18Z

MagneticField/GeomBuilder/src/DD4hep_volumeHandle.cc

+  // For cons and tubs RN = R.
+  theRN = rCentr;
+
+  const double epsilon = 1e-5;


could also be static constexpr

jpata · 2020-10-02T09:10:27Z

MagneticField/GeomBuilder/src/DD4hep_volumeHandle.cc

+  Surface::RotationType rot_R(planeZAxis, x_Rplane);
+
+  // FIXME: use builder
+  surfaces[plane_side] = new Plane(pos_Rplane, rot_R);


for my understanding, where are these objects deleted later? Going through the volumeHandle base classes, I was not immediately able to idenfity it, but I wasn't looking in the right place probably.

Surfaces are reference counted. cf. https://github.com/cms-sw/cmssw/blob/master/DataFormats/GeometrySurface/interface/Surface.h#L36

namapane · 2020-10-02T11:32:53Z

@namapane - when your PR is ready, I'll rebase this one.

@ianna, you can proceed with rebasing on top of #31649. Thanks.

ianna · 2020-10-06T15:18:00Z

+1

ianna · 2020-10-06T15:18:20Z

hold

cmsbuild · 2020-10-06T15:18:23Z

Pull request has been put on hold by @ianna
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

ianna · 2020-10-07T07:58:58Z

replaced by #31691

ianna added 5 commits September 25, 2020 10:24

use dd4hep shape accessors

21c69bf

remove wrapper shape classes

f275921

Filtered view is not a shape

582f2ba

Merged dd4hep-mf-geometry-to-use-dd4hep-shapes-v1 from repository ian…

8145622

…na with cms-merge-topic

code format

05dee07

cmsbuild added this to the CMSSW_11_2_X milestone Sep 28, 2020

cmsbuild added code-checks-pending comparison-pending geometry-pending orp-pending pending-signatures reconstruction-pending tests-pending upgrade-pending labels Sep 28, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 28, 2020

cmsbuild added tests-started and removed tests-pending labels Sep 28, 2020

cmsbuild added tests-rejected and removed tests-started labels Sep 28, 2020

cmsbuild added requires-external tests-started labels Sep 28, 2020

cmsbuild added tests-approved and removed tests-started labels Sep 28, 2020

cmsbuild added comparison-available and removed comparison-pending labels Sep 28, 2020

This was referenced Oct 1, 2020

DD4hep: Introduces functional Tracker navigation + DetIds building #31573

Merged

[DD4hep] Filtered View Performance - Step 2 #31641

Merged

cmsbuild added upgrade-approved and removed upgrade-pending labels Oct 1, 2020

jpata reviewed Oct 2, 2020

View reviewed changes

namapane mentioned this pull request Oct 2, 2020

Remove dependency from DDCMS/interface/DDShapes.h #31649

Merged

This was referenced Oct 2, 2020

[DD4hep] Remove Spurious Include #31652

Merged

Phase2-hgx263 Try to resolve the orientation index issue for HGCal V13/V14 #31653

Merged

[DD4hep] Filtered View Performance - Step 3 #31662

Merged

cmsbuild added geometry-approved hold and removed geometry-pending labels Oct 6, 2020

cmsbuild mentioned this pull request Oct 7, 2020

[DD4hep] Filtered View Performance - Step 4 #31690

Merged

ianna mentioned this pull request Oct 7, 2020

[DD4hep] Filtered View is not a Shape #31691

Merged

ianna closed this Oct 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DD4hep] Filtered View is not a Shape #31603

[DD4hep] Filtered View is not a Shape #31603

ianna commented Sep 28, 2020

cmsbuild commented Sep 28, 2020

ianna commented Sep 28, 2020

cmsbuild commented Sep 28, 2020

cmsbuild commented Sep 28, 2020 •

edited

cmsbuild commented Sep 28, 2020

namapane commented Sep 28, 2020

ianna commented Sep 28, 2020

cmsbuild commented Sep 28, 2020

cmsbuild commented Sep 28, 2020

cmsbuild commented Sep 28, 2020

cmsbuild commented Sep 28, 2020

cmsbuild commented Sep 28, 2020

cmsbuild commented Sep 28, 2020

kpedro88 commented Oct 1, 2020

jpata left a comment

jpata Oct 2, 2020

jpata Oct 2, 2020

namapane Oct 2, 2020

namapane commented Oct 2, 2020

ianna commented Oct 6, 2020

ianna commented Oct 6, 2020

cmsbuild commented Oct 6, 2020

ianna commented Oct 7, 2020

[DD4hep] Filtered View is not a Shape #31603

[DD4hep] Filtered View is not a Shape #31603

Conversation

ianna commented Sep 28, 2020

PR description:

PR validation:

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

cmsbuild commented Sep 28, 2020

ianna commented Sep 28, 2020

cmsbuild commented Sep 28, 2020

cmsbuild commented Sep 28, 2020 • edited

cmsbuild commented Sep 28, 2020

namapane commented Sep 28, 2020

ianna commented Sep 28, 2020

cmsbuild commented Sep 28, 2020

cmsbuild commented Sep 28, 2020

cmsbuild commented Sep 28, 2020

cmsbuild commented Sep 28, 2020

cmsbuild commented Sep 28, 2020

cmsbuild commented Sep 28, 2020

kpedro88 commented Oct 1, 2020

jpata left a comment

Choose a reason for hiding this comment

jpata Oct 2, 2020

Choose a reason for hiding this comment

jpata Oct 2, 2020

Choose a reason for hiding this comment

namapane Oct 2, 2020

Choose a reason for hiding this comment

namapane commented Oct 2, 2020

ianna commented Oct 6, 2020

ianna commented Oct 6, 2020

cmsbuild commented Oct 6, 2020

ianna commented Oct 7, 2020

cmsbuild commented Sep 28, 2020 •

edited