Revert "Port CTPPS to DD4hep" #31359

davidlange6 · 2020-09-04T06:53:54Z

Reverts #31240
several problems have been identified in c++ dependencies and python due to this PR.

This reverts commit 499518f.

cmsbuild · 2020-09-04T06:54:19Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-09-04T07:01:47Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31359/18153

This PR adds an extra 8KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File CondFormats/GeometryObjects/BuildFile.xml modified in PR(s): add missing includes to CondFormats/GeometryObjects #31343
- File DetectorDescription/DDCMS/interface/DDFilteredView.h modified in PR(s): DD4hep: CmsTrackerLevelBuilder Fix Tracker Geometry Navigation #31337
- File Geometry/VeryForwardGeometry/python/geometryIdealPPSFromDD_2016_cfi.py modified in PR(s): [PPS] Timing detector preparation for 2021 conditions #30252
- File Geometry/VeryForwardGeometry/python/geometryIdealPPSFromDD_2017_cfi.py modified in PR(s): [PPS] Timing detector preparation for 2021 conditions #30252
- File Geometry/VeryForwardGeometry/python/geometryIdealPPSFromDD_2018_cfi.py modified in PR(s): [PPS] Timing detector preparation for 2021 conditions #30252
- File Geometry/VeryForwardGeometry/python/geometryRPFromDD_2017_cfi.py modified in PR(s): [PPS] Timing detector preparation for 2021 conditions #30252
- File Geometry/VeryForwardGeometry/python/geometryRPFromDD_2018_cfi.py modified in PR(s): [PPS] Timing detector preparation for 2021 conditions #30252
- File Geometry/VeryForwardGeometryBuilder/plugins/CTPPSGeometryESModule.cc modified in PR(s): [PPS] Timing detector preparation for 2021 conditions #30252
- File Validation/CTPPS/plugins/CTPPSDirectProtonSimulation.cc modified in PR(s): [PPS] Timing detector preparation for 2021 conditions #30252

cmsbuild · 2020-09-04T07:02:08Z

A new Pull Request was created by @davidlange6 (David Lange) for master.

It involves the following packages:

CondFormats/GeometryObjects
CondTools/Geometry
DQM/CTPPS
DetectorDescription/DDCMS
Geometry/VeryForwardData
Geometry/VeryForwardGeometry
Geometry/VeryForwardGeometryBuilder
RecoPPS/Local
SimPPS/RPDigiProducer
Validation/CTPPS

@perrotta, @kmaeshima, @andrius-k, @Dr15Jones, @makortel, @cvuosalo, @civanch, @tlampen, @christopheralanwest, @ianna, @mdhildreth, @cmsbuild, @jpata, @jfernan2, @fioriNTU, @slava77, @ggovi, @pohsun, @tocheng can you please review it and eventually sign? Thanks.
@vargasa, @forthommel, @jan-kaspar, @tocheng, @slomeo, @mmusich, @fabiocos, @seemasharmafnal 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

cmsbuild · 2020-09-04T11:34:09Z

The tests are being triggered in jenkins.
Tested with other pull request(s) #31356

CMSSW_11_2_X_2020-09-04-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/9122/console Started: 2020/09/04 13:34

Dr15Jones · 2020-09-04T12:03:31Z

CondFormats/GeometryObjects/interface/PDetGeomDesc.h


 #include <vector>
 #include <string>

 class PDetGeomDesc {
 public:
-  struct Item {
-    Item() = default;
-    Item(const DetGeomDesc* const geoInfo) {


It looks like one way to break the cyclic dependency is to remove this constructor and replace it with a helper function doing the same operations but in a different package.

eg, where the code was before (as the class member data are public..)

I wonder why this change even went in

A priori is nicer to have things done in the constructor once and for all, than having a constructor not initializing the built-in data members, then pass around an object with non-initialized built-in data members, then finally set the data members to values we already had since the beginning.
Then if there is a circular dependency in the packages, that makes it indeed not worth it, but it is a pity this does not get detected before.
How to test for circular dependencies?

In the previous code for example, as expected, we find later down the line...
PDetGeomDesc* pdet = new PDetGeomDesc;

Ha yes wait the float, int, etc, are data members of Item anyway, not of PDetGeomDesc.

but the way the default constructor of the class whose members are built-in types does not matter for the initialization of these members

I take it a bit back that for a class without a user provided default constructor, whether the constructor is called with or without parentheses actually matters, and does dictate whether the member built-in types are zero-initialized. (the actual initialization rules are a bit complex
https://en.cppreference.com/w/cpp/language/default_initialization
https://en.cppreference.com/w/cpp/language/value_initialization
)

@markotel, yes exactly. Basically if PDetGeomDesc had built-in types, there would not have been zero-initialized.
PDetGeomDesc* pdet = new PDetGeomDesc and PDetGeomDesc* pdet = new PDetGeomDesc() are different.

In general, I tend to do what can be done in the constructor, instead of delaying (in terms of object life timeline) and having this type of situation.

I tend to only alter the object when needed (ie when some info has been computed, which was not already available at construction time).

I think this makes things clearer overall, because one needs to always look when an object is touched, so the less modifications to look at the better.
Imagine a codebase where all objects are zero-intialized at construction (or even not, in the built-in case, which is even problematic), then zillions of setters to modify them. Things quickly become a mess.
So I tend to like constructors ;p

But I understand CondFormats/GeometryObjects is a special package, so we cannot really simply add a constructor there without having a mess. Not worth the effort ;p
In that case, better to call
PDetGeomDesc* pdet = new PDetGeomDesc()
( but not the existing PDetGeomDesc* pdet = new PDetGeomDesc )
and have the settings of all the data members one by one a posteriori.

You can (and I’d argue should) have a constructor, just not one using a DetGeomDesc. Instead, pass the values need to set the actual member data. That guarantees everything is properly initialized and avoids unnecessary coupling of packages.

@Dr15Jones Ha thanks, good idea, that's the cleanest approach.

cmsbuild · 2020-09-04T14:22:06Z

+1
Tested at: bf9b9ec
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8ad442/9122/summary.html
CMSSW: CMSSW_11_2_X_2020-09-04-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-09-04T14:22:11Z

Comparison job queued.

cvuosalo · 2020-09-04T16:09:16Z

Can we wait briefly before merging this PR? @ghugo83 is trying to fix the issues in #31240 and is submitting another PR today. What is the deadline for getting this PR into the next IB?

davidlange6 · 2020-09-04T16:57:33Z

Presumably the numerous fixes would need review, no? On Sep 4, 2020, at 6:09 PM, Carl Vuosalo <notifications@github.com<mailto:notifications@github.com>> wrote: Can we wait briefly before merging this PR? @ghugo83<https://github.com/ghugo83> is trying to fix the issues in #31240<#31240> and is submitting another PR today. What is the deadline for getting this PR into the next IB? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#31359 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABGPFQ2N7FI5QVLBQK53Q4TSEEGL7ANCNFSM4QXOKLUA>.

cvuosalo · 2020-09-04T17:07:37Z

It's probably best to go ahead with this PR now. Revision of #31240 to fix its issues can be done in the next few days.

cvuosalo · 2020-09-04T17:07:44Z

+1

christopheralanwest · 2020-09-04T17:17:27Z

+1

jfernan2 · 2020-09-05T19:35:09Z

+1

qliphy · 2020-09-07T02:14:25Z

@cms-sw/reconstruction-l2 @cms-sw/simulation-l2 @cms-sw/db-l2 any further comment?

davidlange6 · 2020-09-07T07:28:47Z

hi @silviodonato @qliphy - do you plan just to live with the problems introduced in #31240 or is this getting merged?

silviodonato · 2020-09-07T07:31:05Z

hi @silviodonato @qliphy - do you plan just to live with the problems introduced in #31240 or is this getting merged?

hi @davidlange6 I planned to merge this before the morning IB

davidlange6 · 2020-09-07T07:34:53Z

Thanks for the clarification Cheers David On Sep 7, 2020, at 9:31 AM, Silvio Donato <notifications@github.com<mailto:notifications@github.com>> wrote: hi @silviodonato<https://github.com/silviodonato> @qliphy<https://github.com/qliphy> - do you plan just to live with the problems introduced in #31240<#31240> or is this getting merged? hi @davidlange6<https://github.com/davidlange6> I planned to merge this before the morning IB — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#31359 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABGPFQYNPJEN7DZYTMDQKTTSESD4XANCNFSM4QXOKLUA>.

silviodonato · 2020-09-07T08:34:21Z

merge

…S_final" This reverts commit 2c7b8d6, reversing changes made to 3173c7f.

jfernan2 · 2023-09-18T08:18:53Z

+1

Revert "Port CTPPS to DD4hep (#31240)"

bf9b9ec

This reverts commit 499518f.

cmsbuild added this to the CMSSW_11_2_X milestone Sep 4, 2020

cmsbuild added alca-pending code-checks-pending comparison-pending db-pending dqm-pending geometry-pending orp-pending pending-signatures reconstruction-pending simulation-pending tests-pending labels Sep 4, 2020

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

cmsbuild mentioned this pull request Sep 4, 2020

Fix PPS XML parsing issue #31362

Closed

davidlange6 mentioned this pull request Sep 4, 2020

Add missing includes to Geometry/Tracker(Numbering/Geometry)Builder #31356

Merged

cmsbuild added requires-external tests-started and removed tests-pending labels Sep 4, 2020

jpata mentioned this pull request Sep 4, 2020

Port CTPPS to DD4hep #31240

Merged

Dr15Jones reviewed Sep 4, 2020

View reviewed changes

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

cmsbuild added the comparison-available label Sep 4, 2020

cmsbuild added geometry-approved and removed geometry-pending labels Sep 4, 2020

cmsbuild added alca-approved and removed alca-pending labels Sep 4, 2020

This was referenced Sep 4, 2020

Integrating VectorHits reconstruction for the Phase2 OT - reprise #31314

Merged

Clean BuildFiles in Geometry #31368

Merged

cmsbuild added dqm-approved and removed dqm-pending labels Sep 5, 2020

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

cmsbuild merged commit 2c7b8d6 into master Sep 7, 2020

ghugo83 added a commit to ghugo83/cmssw that referenced this pull request Sep 7, 2020

Revert "Merge pull request cms-sw#31359 from cms-sw/revert-31240-CTPP…

581d173

…S_final" This reverts commit 2c7b8d6, reversing changes made to 3173c7f.

ghugo83 added a commit to ghugo83/cmssw that referenced this pull request Sep 7, 2020

Revert "Merge pull request cms-sw#31359 from cms-sw/revert-31240-CTPP…

f7e7868

…S_final" This reverts commit 2c7b8d6, reversing changes made to 3173c7f.

silviodonato mentioned this pull request Sep 8, 2020

add missing includes to CondFormats/GeometryObjects #31343

Closed

smuzaffar deleted the revert-31240-CTPPS_final branch November 6, 2020 21:07

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Sep 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert "Port CTPPS to DD4hep" #31359

Revert "Port CTPPS to DD4hep" #31359

davidlange6 commented Sep 4, 2020

cmsbuild commented Sep 4, 2020

cmsbuild commented Sep 4, 2020

cmsbuild commented Sep 4, 2020

cmsbuild commented Sep 4, 2020 •

edited

Dr15Jones Sep 4, 2020

davidlange6 Sep 4, 2020

ianna Sep 4, 2020

ghugo83 Sep 4, 2020 •

edited

ghugo83 Sep 4, 2020 •

edited

ghugo83 Sep 4, 2020

makortel Sep 4, 2020

ghugo83 Sep 4, 2020 •

edited

Dr15Jones Sep 4, 2020

ghugo83 Sep 5, 2020

cmsbuild commented Sep 4, 2020

cmsbuild commented Sep 4, 2020

cvuosalo commented Sep 4, 2020

davidlange6 commented Sep 4, 2020 via email

cvuosalo commented Sep 4, 2020

cvuosalo commented Sep 4, 2020

christopheralanwest commented Sep 4, 2020

jfernan2 commented Sep 5, 2020

qliphy commented Sep 7, 2020

davidlange6 commented Sep 7, 2020

silviodonato commented Sep 7, 2020

davidlange6 commented Sep 7, 2020 via email

silviodonato commented Sep 7, 2020

jfernan2 commented Sep 18, 2023

Revert "Port CTPPS to DD4hep" #31359

Revert "Port CTPPS to DD4hep" #31359

Conversation

davidlange6 commented Sep 4, 2020

cmsbuild commented Sep 4, 2020

cmsbuild commented Sep 4, 2020

cmsbuild commented Sep 4, 2020

cmsbuild commented Sep 4, 2020 • edited

Dr15Jones Sep 4, 2020

Choose a reason for hiding this comment

davidlange6 Sep 4, 2020

Choose a reason for hiding this comment

ianna Sep 4, 2020

Choose a reason for hiding this comment

ghugo83 Sep 4, 2020 • edited

Choose a reason for hiding this comment

ghugo83 Sep 4, 2020 • edited

Choose a reason for hiding this comment

ghugo83 Sep 4, 2020

Choose a reason for hiding this comment

makortel Sep 4, 2020

Choose a reason for hiding this comment

ghugo83 Sep 4, 2020 • edited

Choose a reason for hiding this comment

Dr15Jones Sep 4, 2020

Choose a reason for hiding this comment

ghugo83 Sep 5, 2020

Choose a reason for hiding this comment

cmsbuild commented Sep 4, 2020

cmsbuild commented Sep 4, 2020

cvuosalo commented Sep 4, 2020

davidlange6 commented Sep 4, 2020 via email

cvuosalo commented Sep 4, 2020

cvuosalo commented Sep 4, 2020

christopheralanwest commented Sep 4, 2020

jfernan2 commented Sep 5, 2020

qliphy commented Sep 7, 2020

davidlange6 commented Sep 7, 2020

silviodonato commented Sep 7, 2020

davidlange6 commented Sep 7, 2020 via email

silviodonato commented Sep 7, 2020

jfernan2 commented Sep 18, 2023

cmsbuild commented Sep 4, 2020 •

edited

ghugo83 Sep 4, 2020 •

edited

ghugo83 Sep 4, 2020 •

edited

ghugo83 Sep 4, 2020 •

edited