Migration to DD4hep: Geometric(Timing)Det dependence on DDD

[fabiocos]

In the process of migrating the MTD reco geometry to DD4hep, I notice that the GeometricTimingDet object in MTDNumberingBuilder, as well as its tracker counterpart GeometricDet, depend explicitly on DDD through DDSolidShape and the use of the ExpandedNode. This means that even the present tracker migration is not fully complete.

Before inventing ad-hoc solutions, I would like to understand from @ianna and @cvuosalo whether they already envisaged a strategy to manage this issue.

@vargasa FYI

[cmsbuild]

A new Issue was created by @fabiocos Fabio Cossutti.

@Dr15Jones, @smuzaffar, @silviodonato, @makortel, @davidlange6, @fabiocos can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[fabiocos]

assign geometry

[cmsbuild]

New categories assigned: geometry

@Dr15Jones,@cvuosalo,@mdhildreth,@makortel,@ianna,@civanch you have been requested to review this Pull request/Issue and eventually sign? Thanks

[ianna]

@cvuosalo and @fabiocos - following the discussion with Adinda here.
Given that we want to support the legacy payloads and due to a recorded int as a shape identifier in a PGeometricDet we need to make sure that the old and the new DDSolidShape enums are identical. It's not the case now:
https://cmssdt.cern.ch/lxr/source/DetectorDescription/Core/interface/DDSolidShapes.h#0006

0006 enum class DDSolidShape {
0007   dd_not_init = 0,
0008   ddbox = 1,
0009   ddtubs = 2,
0010   ddtrap = 3,
0011   ddcons = 4,
0012   ddpolycone_rz = 5,
0013   ddpolyhedra_rz = 6,
0014   ddpolycone_rrz = 7,
0015   ddpolyhedra_rrz = 8,
0016   ddtorus = 9,
0017   ddunion = 10,
0018   ddsubtraction = 11,
0019   ddintersection = 12,
0020   ddshapeless = 13,
0021   ddpseudotrap = 14,
0022   ddtrunctubs = 15,
0023   ddsphere = 16,
0024   ddellipticaltube = 17,
0025   ddcuttubs = 18,
0026   ddextrudedpolygon = 19,
0027 };
0028

https://cmssdt.cern.ch/lxr/source/DetectorDescription/DDCMS/interface/DDSolidShapes.h#0062

0062  enum class DDSolidShape {
0063     dd_not_init = 0,
0064     ddbox = 1,
0065     ddtubs = 2,
0066     ddtrap = 3,
0067     ddcons = 4,
0068     ddpolycone = 5,
0069     ddpolyhedra = 6,
0070     ddtorus = 7,
0071     ddunion = 8,
0072     ddsubtraction = 9,
0073     ddintersection = 10,
0074     ddshapeless = 11,
0075     ddpseudotrap = 12,
0076     ddtrunctubs = 13,
0077     ddsphere = 14,
0078     ddellipticaltube = 15,
0079     ddcuttubs = 16,
0080     ddextrudedpolygon = 17,
0081     ddtrd1 = 18,
0082   };
0083 

[fabiocos]

@ianna thanks for looking into this. Silly question: what was the reason for this choice at the beginning of the porting? I mean I see that the two lists mostly overlaps, and codes could be made correspondent, with 2 o3 noticeable differences. Do we have loops that are preventing discontinuities in the lists? The question is of course about the correspondence of polycones and polyhedras...

[ianna]

@ianna thanks for looking into this. Silly question: what was the reason for this choice at the beginning of the porting? I mean I see that the two lists mostly overlaps, and codes could be made correspondent, with 2 o3 noticeable differences. Do we have loops that are preventing discontinuities in the lists? The question is of course about the correspondence of polycones and polyhedras...

Changing enum was intentional to weed out reliance on int.

[fabiocos]

@ianna sorry, perhaps I was not clear. I am not questioning the enum choice, just wondering about the reason for the two different structures.

[cvuosalo]

@fabiocos Adinda is fixing the DDSolidShape issue in these files, but the DDExpandedView part still needs to be migrated. What are your plans for migrating these files?

[fabiocos]

@cvuosalo are you speaking of
https://github.com/cms-sw/cmssw/blob/master/Geometry/MTDNumberingBuilder/interface/GeometricTimingDet.h#L41
https://github.com/cms-sw/cmssw/blob/master/Geometry/MTDNumberingBuilder/interface/GeometricTimingDet.h#L41
?

This dependence seems completely fake to me, given that it is just a common type definition:
https://github.com/cms-sw/cmssw/blob/master/DetectorDescription/Core/interface/DDExpandedView.h#L48

and we might either put this common definition somewhere else or just cut-and-paste it explicitly (I do not see anything thrilling in a vector of integers).

As far as PoolAlloc is concerned, I do not see any definition or use of it within CMSSW, is this a remnant of the past?

[cvuosalo]

@fabiocos Actually, Adinda has just fixed the nav_type in #29905. There is still the using GeoHistory = std::vector<DDExpandedNode>; to remove or fix in several MTD and Tracker files.
I think PoolAlloc is obsolete and can be deleted.
Could you and @adewit please coordinate on how these fixes can be made?

[adewit]

Hi, I had another look at the DDExpandedNode usage in the code today to see if it might be straightforward to do these fixes. I see that DDFilteredView has a geoHistory method which returns a vector of DDExpandedNodes. It looks like in cms::DDFilteredView that has been replaced by a method that returns a vector of TGeoNodes - I'm afraid I don't have time to try to understand how this is different from what was used in the old DD case (and thus how any methods called on GeoHistorys would have to be modified)

[cvuosalo]

@adewit, @fabiocos, @vargasa The DD4hep migration of the TrackerNumberingBuilder won't be complete until the dependence on DDExpandedNode is handled. Can one of you perform this task?

[fabiocos]

@adewit see my comments in #30276 (comment) In my understanding GeometricDetExtra is just a debugging tool used in a very limited number of places. Is it still actively used by the Tracker community?

Assuming the answering is yes, in my view the issue is not how to extract the GeoHistory information used from cms::DDFilteredView, because for that I believe one could just use that history in a manner similar to what I do in my MTD test code, where I build the whole volume hierarchy above a given node. The issue is more linked to the fact that one of the data members of this class is a DDD object, and it cannot be kept as it is if you want to become independent of it. One simple possibility is to replace it with a structure of arrays, one containing volume names and the other corresponding copy numbers (what I call basenames) encoding the information that is practically used for what I can see in the debugging code.

[adewit]

Adding @mmusich since he probably knows this better than me. I agree it looks like GeometricDetExtra is just used for debugging in the tracker code, but someone might need the plugins that use it anyway.

[cvuosalo]

PR #29905 has migrated the Tracker Numbering Builder GeometricDet objects to DD4hep. Now the same changes need to be made to the MTD Numbering Builder.

[cvuosalo]

MTD Numbering Builder migration has been completed with the merging of #30834. This issue can be closed.

[cvuosalo]

+1

[cmsbuild]

This issue is fully signed and ready to be closed.