-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DD4Hep migration (SolidShapes) for TrackerNumberingBuilder (+TkAl & related CondTool/Geometry plugins) #29905
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29905/15496
|
A new Pull Request was created by @adewit for master. It involves the following packages: Alignment/OfflineValidation @civanch, @Dr15Jones, @makortel, @cvuosalo, @christopheralanwest, @ianna, @mdhildreth, @cmsbuild, @tocheng, @tlampen, @ggovi, @pohsun, @kpedro88 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
The tests are being triggered in jenkins. |
+1 |
Comparison job queued. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few questions about this update, which is interesting also for MTD, where I should forward port it
@@ -360,7 +361,7 @@ void TrackerGeometryCompare::createROOTGeometry(const edm::EventSetup& iSetup) { | |||
} | |||
|
|||
//accessing the initial geometry | |||
edm::ESTransientHandle<DDCompactView> cpv; | |||
edm::ESTransientHandle<cms::DDCompactView> cpv; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fabio is right. We want both. An example of how to handle both old DD and DD4hep in the same file is here:
https://cmssdt.cern.ch/lxr/source/Geometry/MTDGeometryBuilder/plugins/MTDParametersESModule.cc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed for the plugins now
@@ -10,11 +10,15 @@ | |||
#include "CondFormats/GeometryObjects/interface/PGeometricDet.h" | |||
#include "Geometry/Records/interface/IdealGeometryRecord.h" | |||
#include "Geometry/TrackerNumberingBuilder/interface/GeometricDet.h" | |||
#include "DetectorDescription/Core/interface/DDCompactView.h" | |||
#include "DetectorDescription/DDCMS/interface/DDCompactView.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here ase well
@@ -10,7 +10,7 @@ | |||
#include "CondFormats/GeometryObjects/interface/PGeometricDetExtra.h" | |||
#include "Geometry/Records/interface/PGeometricDetExtraRcd.h" | |||
#include "Geometry/TrackerNumberingBuilder/interface/GeometricDetExtra.h" | |||
#include "DetectorDescription/Core/interface/DDCompactView.h" | |||
#include "DetectorDescription/DDCMS/interface/DDCompactView.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here as well
@@ -5,7 +5,7 @@ | |||
#include "FWCore/ServiceRegistry/interface/Service.h" | |||
#include "CondCore/DBOutputService/interface/PoolDBOutputService.h" | |||
#include "CondFormats/GeometryObjects/interface/PTrackerParameters.h" | |||
#include "DetectorDescription/Core/interface/DDCompactView.h" | |||
#include "DetectorDescription/DDCMS/interface/DDCompactView.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here as well
@@ -223,5 +223,5 @@ GeometricTimingDet::Rotation GeometricTimingDet::rotationBounds() const { | |||
std::unique_ptr<Bounds> GeometricTimingDet::bounds() const { | |||
const std::vector<double>& par = params_; | |||
TrackerShapeToBounds shapeToBounds; | |||
return std::unique_ptr<Bounds>(shapeToBounds.buildBounds(shape_, par)); | |||
return std::unique_ptr<Bounds>(shapeToBounds.buildBounds(cms::DDSolidShape(static_cast<int>(shape_)), par)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, thinking about it again I think you're right that this would break down if we had a ddpolycone_rrz/ddpolyhedra_rrz (in old dd) - the corresponding int would be 7 or 8 and that doesn't give a corresponding type in cms::DDSolidShape. Think there is a map to the single ddpolycone/ddpolyhedra that can be used to get this correct, need to check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -10,7 +10,8 @@ | |||
|
|||
#include "Geometry/Records/interface/IdealGeometryRecord.h" | |||
#include "Geometry/TrackerNumberingBuilder/interface/GeometricDet.h" | |||
#include <DetectorDescription/Core/interface/DDCompactView.h> | |||
//#include <DetectorDescription/Core/interface/DDCompactView.h> | |||
#include <DetectorDescription/DDCMS/interface/DDCompactView.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again this looks a simple replacement
return buildOpen(_par); | ||
break; | ||
case DDSolidShape::ddpolycone_rrz: | ||
case cms::DDSolidShape::ddpolycone: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code appears to do more than it actually does. It should be simplified for clarity.
case cms::DDSolidShape::ddtubs:
case cms::DDSolidShape::ddpolycone:
case cms::DDSolidShape::ddsubtraction:
return buildOpen();
break;
Note that buildOpen()
should be changed to take no parameter. If you check the definition of the method, it doesn't use its parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @adewit, you may need to double check with @cvuosalo. It looks to me the changes are made directly and as I understand there should be a way to switch between Core
/DDCMS
(*) until the migration is completed/validated. If the changes were already validated it should be fine tho (imo) but it is good to double check. Thank you for your work :)
[*] http://cms.cern.ch/iCMS/jsp/openfile.jsp?tp=draft&files=8001_cmsdd4hep.pdf
@@ -10,7 +10,8 @@ | |||
|
|||
#include "Geometry/Records/interface/IdealGeometryRecord.h" | |||
#include "Geometry/TrackerNumberingBuilder/interface/GeometricDet.h" | |||
#include <DetectorDescription/Core/interface/DDCompactView.h> | |||
//#include <DetectorDescription/Core/interface/DDCompactView.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If headers are not needed anymore please remove them
@@ -37,7 +38,7 @@ void GeometricDetLoader::beginRun(edm::Run const& /* iEvent */, edm::EventSetup | |||
std::cout << "PoolDBOutputService unavailable" << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When possible use LogVerbatim
. see #29610
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a load of couts in all these files - maybe changing them over to LogVerbatim
should be decoupled from this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adewit Yes, the cout
fix can be done later.
@@ -4,6 +4,7 @@ | |||
#include "CondFormats/GeometryObjects/interface/PGeometricDet.h" | |||
#include "DetectorDescription/Core/interface/DDExpandedView.h" | |||
#include "DetectorDescription/Core/interface/DDSolidShapes.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core
headers, see my comment above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Andres is right. There are still problems here. I think line 6 needs to be deleted. I don't see the old DDSolidShape
being used here.
Also, migration code has to provided for the old DDExpandedView
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha - I missed this. L6 can indeed go. As for DDExpandedView
- what it provides for this file is two types that are effectively vectors of ints, so I've moved those defs into this file. However, I also see using GeoHistory = std::vector<DDExpandedNode>;
, even though GeoHistory
is not used in this file. Perhaps that can be removed - then DDExpandedView
is not needed in this file at all.
I assume there is a good reason there's no direct dd4hep copy of DDExpandedView
/ DDExpandedNode
(yet) - but if central changes are needed there they could then happen outside of this PR. In any case I don't think I'm the right person to make those changes (I definitely don't know enough about the requirements to be able to do this correctly)
Comparison is ready Comparison Summary:
|
The tests are being triggered in jenkins.
|
+1 |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
+1 I performed tests similar to those in #29905 (comment). For the dumped versions of the tracker reco geometry, the only differences in the content of the |
I verified that this PR introduces no change in the output of the MTD geometry tests (extended version, where the solid shape is printed as well) |
@ggovi Please review this PR. |
+1 |
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 (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
This changes:
PR validation:
Unit tests run successfully; for tracker alignment plugin output was compared before and after these changes
if this PR is a backport please specify the original PR and why you need to backport that PR:
Not a backport