PPS: code to copy-without-children moved from copy constructor to a named method #31492

jan-kaspar · 2020-09-17T10:18:11Z

PR description:

This is a technical PR to address issue #25434. The code to perform a shall copy (without children) is moved from copy constructor to a named method to avoid misunderstandings.

No change in results is expected due to this PR.

PR validation:

The two plots below (top for "direct" simu, bottom for reco) document that there is indeed no change due to this PR.

method.

cmsbuild · 2020-09-17T10:18:35Z

The code-checks are being triggered in jenkins.

jan-kaspar · 2020-09-17T10:18:40Z

FYI @forthommel @ghugo83

cmsbuild · 2020-09-17T10:24:30Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31492/18428

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File Geometry/VeryForwardGeometryBuilder/interface/DetGeomDesc.h modified in PR(s): [PPS] 2021 conditions for timing detectors geometry (resubmission after DD4hep) #31484
- File Geometry/VeryForwardGeometryBuilder/plugins/CTPPSGeometryESModule.cc modified in PR(s): [PPS] 2021 conditions for timing detectors geometry (resubmission after DD4hep) #31484
- File Geometry/VeryForwardGeometryBuilder/src/DetGeomDesc.cc modified in PR(s): [PPS] 2021 conditions for timing detectors geometry (resubmission after DD4hep) #31484

cmsbuild · 2020-09-17T10:24:56Z

A new Pull Request was created by @jan-kaspar 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, @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

makortel · 2020-09-17T12:49:32Z

Geometry/VeryForwardGeometryBuilder/interface/DetGeomDesc.h

  virtual ~DetGeomDesc();

+  DetGeomDesc* makeCopyWithoutChildren() const;


Can this be made to return std::unique_ptr<DetGeomDesc> to make it explicit that the caller gets the ownership?

Looks like a return by value would work as well. If so, that is better than returning by pointer or unique_ptr.

The current code works with pointers:

cmssw/Geometry/VeryForwardGeometryBuilder/interface/DetGeomDesc.h

Line 52 in 0e26c4a

using Container = std::vector<DetGeomDesc*>;

cmssw/Geometry/VeryForwardGeometryBuilder/plugins/CTPPSGeometryESModule.cc

Line 131 in 0e26c4a

DetGeomDesc* alignedDetRoot = idealDetRoot.makeCopyWithoutChildren();

thus I proposed a function returning a pointer. Switching to unique_ptr is trivial. Let me know what your suggestion is.

One option would be to add a (private) constructor that takes all

cmssw/Geometry/VeryForwardGeometryBuilder/src/DetGeomDesc.cc

Lines 65 to 76 in 0e26c4a

output->m_name = m_name;

output->m_copy = m_copy;

output->m_isDD4hep = m_isDD4hep;

output->m_trans = m_trans;

output->m_rot = m_rot;

output->m_params = m_params;

output->m_isABox = m_isABox;

output->m_diamondBoxParams = m_diamondBoxParams;

output->m_sensorType = m_sensorType;

output->m_geographicalID = m_geographicalID;

output->m_z = m_z;

as parameters.

I was thinking it might be better to remove makCopyWithoutChildren and create a constructor that takes a DetGeomDesc const& and a 'dummy' enum with the only value noChildren. Then the calling code would become

DetGeomDesc* alignedDetRoot = new DetGeomDesc(idealDetRoot, DetGeomDesc::noChildren);

@Dr15Jones Yes exactly
+10000

Many thanks for all the suggestions! I also like this one. I will give it a try tomorrow.

ghugo83 · 2020-09-17T12:58:18Z

Geometry/VeryForwardGeometryBuilder/interface/DetGeomDesc.h

@@ -53,15 +53,17 @@ class DetGeomDesc {
  using RotationMatrix = ROOT::Math::Rotation3D;
  using Translation = ROOT::Math::DisplacementVector3D<ROOT::Math::Cartesian3D<double>>;

+  DetGeomDesc() = default;


Why adding the default constructor?
A DetGeomDesc built with this, will not have interesting data members, and no way of setting them.

A way of creating an "empty" object is needed here:
https://github.com/cms-sw/cmssw/pull/31492/files/0e26c4a0f7658a37176349a8e446492503cdf81c#diff-cae30eec6c3de96aa4e668edf94cf7a8R62
Would you have a better suggestion?

Make it private?

I think there should just be a custom copy constructor.
This in a sense is just 'trying to have' a constructor, with settings done in makeCopyWithoutChildren()

ghugo83 · 2020-09-17T12:59:43Z

Geometry/VeryForwardGeometryBuilder/src/DetGeomDesc.cc

-  m_geographicalID = ref.m_geographicalID;
-  m_z = ref.m_z;
-  return (*this);
+DetGeomDesc* DetGeomDesc::makeCopyWithoutChildren() const {


Yes the renaming is a good idea

cmsbuild · 2020-09-18T08:44:21Z

The code-checks are being triggered in jenkins.

jan-kaspar · 2020-09-18T08:45:27Z

4e4ac6d implements Dr15Jones 's suggestion #31492 (comment)

cmsbuild · 2020-09-18T08:50:05Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31492/18455

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File Geometry/VeryForwardGeometryBuilder/interface/DetGeomDesc.h modified in PR(s): [PPS] 2021 conditions for timing detectors geometry (resubmission after DD4hep) #31484
- File Geometry/VeryForwardGeometryBuilder/plugins/CTPPSGeometryESModule.cc modified in PR(s): [PPS] 2021 conditions for timing detectors geometry (resubmission after DD4hep) #31484
- File Geometry/VeryForwardGeometryBuilder/src/DetGeomDesc.cc modified in PR(s): [PPS] 2021 conditions for timing detectors geometry (resubmission after DD4hep) #31484

cmsbuild · 2020-09-18T08:50:31Z

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

civanch · 2020-09-18T09:04:41Z

please test

cmsbuild · 2020-09-18T09:05:06Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-09-17-2300/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/9421/console Started: 2020/09/18 11:06

cmsbuild · 2020-09-18T10:23:08Z

+1
Tested at: 4e4ac6d
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f53611/9421/summary.html
CMSSW: CMSSW_11_2_X_2020-09-17-2300
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-09-18T10:23:11Z

Comparison job queued.

cmsbuild · 2020-09-18T12:23:54Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f53611/9421/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: 2540471
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2540448
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

cvuosalo · 2020-09-21T21:37:46Z

+1

cmsbuild · 2020-09-21T21:38:13Z

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-09-22T00:14:33Z

+1

Code to copy-without-children moved from copy constructor to a named

0e26c4a

method.

cmsbuild added this to the CMSSW_11_2_X milestone Sep 17, 2020

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

jan-kaspar mentioned this pull request Sep 17, 2020

CTPPS DetGeomDesc review #25434

Closed

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

makortel reviewed Sep 17, 2020

View reviewed changes

ghugo83 reviewed Sep 17, 2020

View reviewed changes

Introduce copy constructor with copy options.

4e4ac6d

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

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

cmsbuild removed the tests-pending label Sep 18, 2020

cmsbuild added the tests-started label Sep 18, 2020

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

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

cmsbuild mentioned this pull request Sep 18, 2020

HeavyIon Reaction Plane MiniAOD #31129

Merged

cmsbuild added fully-signed geometry-approved and removed geometry-pending pending-signatures labels Sep 21, 2020

cmsbuild added orp-approved and removed orp-pending labels Sep 22, 2020

cmsbuild merged commit 050b6f0 into cms-sw:master Sep 22, 2020

cmsbuild mentioned this pull request Sep 22, 2020

[DD4hep] DDFilteredView API Enhancement and Tracker Configuration by Era #31482

Closed

jan-kaspar deleted the pps_assignment_fix branch September 30, 2020 09:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PPS: code to copy-without-children moved from copy constructor to a named method #31492

PPS: code to copy-without-children moved from copy constructor to a named method #31492

jan-kaspar commented Sep 17, 2020

cmsbuild commented Sep 17, 2020

jan-kaspar commented Sep 17, 2020

cmsbuild commented Sep 17, 2020

cmsbuild commented Sep 17, 2020

makortel Sep 17, 2020 •

edited

Dr15Jones Sep 17, 2020

jan-kaspar Sep 17, 2020

makortel Sep 17, 2020

Dr15Jones Sep 17, 2020

ghugo83 Sep 17, 2020

jan-kaspar Sep 17, 2020

ghugo83 Sep 17, 2020

jan-kaspar Sep 17, 2020

Dr15Jones Sep 17, 2020

ghugo83 Sep 17, 2020 •

edited

ghugo83 Sep 17, 2020

cmsbuild commented Sep 18, 2020

jan-kaspar commented Sep 18, 2020

cmsbuild commented Sep 18, 2020

cmsbuild commented Sep 18, 2020

civanch commented Sep 18, 2020

cmsbuild commented Sep 18, 2020 •

edited

cmsbuild commented Sep 18, 2020

cmsbuild commented Sep 18, 2020

cmsbuild commented Sep 18, 2020

cvuosalo commented Sep 21, 2020

cmsbuild commented Sep 21, 2020

qliphy commented Sep 22, 2020

		virtual ~DetGeomDesc();

		DetGeomDesc* makeCopyWithoutChildren() const;

	output->m_name = m_name;
	output->m_copy = m_copy;
	output->m_isDD4hep = m_isDD4hep;
	output->m_trans = m_trans;
	output->m_rot = m_rot;
	output->m_params = m_params;
	output->m_isABox = m_isABox;
	output->m_diamondBoxParams = m_diamondBoxParams;
	output->m_sensorType = m_sensorType;
	output->m_geographicalID = m_geographicalID;

	output->m_z = m_z;

PPS: code to copy-without-children moved from copy constructor to a named method #31492

PPS: code to copy-without-children moved from copy constructor to a named method #31492

Conversation

jan-kaspar commented Sep 17, 2020

PR description:

PR validation:

cmsbuild commented Sep 17, 2020

jan-kaspar commented Sep 17, 2020

cmsbuild commented Sep 17, 2020

cmsbuild commented Sep 17, 2020

makortel Sep 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghugo83 Sep 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Sep 18, 2020

jan-kaspar commented Sep 18, 2020

cmsbuild commented Sep 18, 2020

cmsbuild commented Sep 18, 2020

civanch commented Sep 18, 2020

cmsbuild commented Sep 18, 2020 • edited

cmsbuild commented Sep 18, 2020

cmsbuild commented Sep 18, 2020

cmsbuild commented Sep 18, 2020

cvuosalo commented Sep 21, 2020

cmsbuild commented Sep 21, 2020

qliphy commented Sep 22, 2020

makortel Sep 17, 2020 •

edited

ghugo83 Sep 17, 2020 •

edited

cmsbuild commented Sep 18, 2020 •

edited