Skip to content
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

refactor!: Build DD4hep geometry from hierarchy, use VariantParameters #1257

Merged
merged 43 commits into from Aug 3, 2022

Conversation

paulgessinger
Copy link
Member

@paulgessinger paulgessinger commented May 17, 2022

This PR changes the DD4hep integration model. Instead of the DD4hep factory code having to depend on ACTS or the ActsDD4hep glue lib, they now use a mechanism we merged into DD4hep (as of 1.21): MapStringVariantStruct, which is wrapped into a detector element extension as VariantParameters.

This extension is independent of ACTS and encodes key value pairs, with the keys being strings and the values being any of string, int, float or bool. ACTS will use information attached to detector elements via this mechanism to identify layers, surface axes and material properties (layer and boundary).

This PR also removes the need to active subdetectors to be marked up using a specific extension: using the DD4hep DetType enum, ACTS will look for detector elements flagged TRACKER and BARREL, ENDCAP or BEAMPIPE and handle them accordingly. Passive structures that cannot be flagged with the above use a variant parameter passive_layer to tell ACTS to translate them.

Possible parameters ACTS will interpret

ACTS geometry translation uses parameters attached to DD4hep detector elements via the VariantParameters extension. You can set this extension from your factory code, or use DD4hep's plugin mechanism to set parameters on detector elements via name. Here's an excerpt from the Open Data Detector on how to use this to mark a passive structure as a passive layer:

<lccdd>
  <detectors>
    <detector id="ODD_Solenoid_ID" name="Solenoid" type="ODDCylinder" beampipe="false" vis="Aluminum">
      <type_flags type="DetType_TRACKER" />
      <boundary_material surface="inner" binning="binPhi,binZ" bins0="mat_sol_bPhi" bins1="mat_sol_bZ"/>
      <tubs name="Solenoid" rmin="sol_rmin" rmax="sol_rmax" dz="sol_hlength" material="Aluminum">
        <layer_material surface="representing" binning="binPhi,binZ" bins0="mat_sol_bPhi" bins1="mat_sol_bZ"/>
      </tubs>
    </detector>
  </detectors>

  <plugins>
    <plugin name="DD4hep_ParametersPlugin">
      <argument value="Solenoid"/>
      <argument value="passive_layer: bool = true"/>
    </plugin>	
  </plugins>
</lccdd>
  • Layer

    • envelope_{r,z}_{min,max}: explicit envelope for a layer

    • layer_material: mark a layer as passive (use this if you want to add a passive layer that is not a beampipe)

    • Surface binning:

      • surface_binning: set to true to indicate that explicit surface binning is set.
      • surface_binning_n_{phi,r}: surface binning for a layer
    • Layer material:

      • layer_material: set to true to indicate that the layer has layer material configuration

        • layer_material_representing: set to true to indicate representing layer material
        • layer_material_inner: set to true to indicate inner layer material
        • layer_material_outer: set to true to indicate outer layer material
        • layer_material_{representing,inner,outer}_bin{X,Y,Z,R,Phi,RPhi,H,Eta,Mag} to give the number of bins in a direction
  • Sensor

    • axis_definitions: local axis definitions for a sensor. Default: XYZ. See {class}Acts::DD4hepDetectorElement for details
  • Volume / subdetector

    • boundary_material: set to true to indicate boundary material is set
    • boundary_material_{negative,positive,inner,outer} to indicate which boundary material surfaces should be set
    • boundary_material_{negative,positive,inner,outer}_bin{X,Y,Z,R,Phi,RPhi,H,Eta,Mag} to give the number of bins in a direction

@paulgessinger paulgessinger added the 🚧 WIP Work-in-progress label May 17, 2022
@paulgessinger paulgessinger added this to the next milestone May 17, 2022
@codecov
Copy link

codecov bot commented May 17, 2022

Codecov Report

Merging #1257 (42dda12) into main (aab7f0c) will decrease coverage by 0.02%.
The diff coverage is 16.66%.

❗ Current head 42dda12 differs from pull request most recent head acdf08c. Consider uploading reports for the commit acdf08c to get more accurate results

@@            Coverage Diff             @@
##             main    #1257      +/-   ##
==========================================
- Coverage   47.49%   47.46%   -0.03%     
==========================================
  Files         375      375              
  Lines       19824    19842      +18     
  Branches     9282     9293      +11     
==========================================
+ Hits         9415     9418       +3     
- Misses       4035     4045      +10     
- Partials     6374     6379       +5     
Impacted Files Coverage Δ
Core/src/Geometry/CylinderVolumeBuilder.cpp 24.01% <16.66%> (-0.47%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@paulgessinger
Copy link
Member Author

Ok, so the python level tests and ROOT hashes seem to be happy, as is the physmon job.

I think this means that the geometry is identical with this PR to main at least as far as the test jobs can tell.

The remaining failing jobs are compilation failures because the DD4hep version in them doesn't have the required changes.

@asalzburger asalzburger self-requested a review July 20, 2022 15:16
@paulgessinger
Copy link
Member Author

DD4hep tag v1.21 exists now, I'll update this PR with new images.

Core/src/Geometry/CylinderVolumeBuilder.cpp Show resolved Hide resolved
Plugins/DD4hep/src/ConvertDD4hepDetector.cpp Show resolved Hide resolved
Plugins/DD4hep/src/ConvertDD4hepDetector.cpp Show resolved Hide resolved
docs/examples/howto/howto.rst Outdated Show resolved Hide resolved
kodiakhq bot pushed a commit that referenced this pull request Jul 25, 2022
In preparation for #1257, this PR removes the old DD4hep detector examples that we don't plan to maintain going forward.
@paulgessinger
Copy link
Member Author

Ok, in the standalone ODD CI I get histogram differences. Marking this WIP again until I've investigated.

@paulgessinger paulgessinger added 🚧 WIP Work-in-progress and removed 🚧 WIP Work-in-progress labels Jul 25, 2022
@paulgessinger
Copy link
Member Author

Differences are understood to have been related to #1298.

@paulgessinger
Copy link
Member Author

@paulgessinger paulgessinger changed the title refactor!: dd4hep from hierarchy refactor!: Build DD4hep geometry from hierarchy, use VariantParameters Jul 27, 2022
@asalzburger
Copy link
Contributor

Paul, there are conflicts here, can you update - otherwise you addressed my comments.

@paulgessinger
Copy link
Member Author

@asalzburger this now points at ODD v3.0.0 and is ready to go. Can you approve?

@asalzburger
Copy link
Contributor

Updated branch, and will approve when done.

@asalzburger asalzburger self-requested a review August 3, 2022 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants