ZDC_Sampling: share layer/slice template volumes across repetitions - #1121
Open
wdconinc wants to merge 2 commits into
Open
ZDC_Sampling: share layer/slice template volumes across repetitions#1121wdconinc wants to merge 2 commits into
wdconinc wants to merge 2 commits into
Conversation
Replace per-repeat unique volume construction with shared templates. Previously the repeat loop in ZeroDegreeCalorimeterSampling_geo.cpp created a new layer_vol (named _layer1, _layer2, ...) and new slice_vol objects (named _layer1slice1, ...) on every iteration. For ZDCHcal (ZDC_PbScint_NRepeat=120, 3 slices per layer) this inflated the TGeo volume registry with 480 unique TGeoVolumes for identical geometry. After this change: - One layer template TGeoVolume is built per layer group. - Three slice TGeoVolumes (one per slice type) are placed in the template once. - The explicit repeat loop reuses that single template 120 times, reducing unique volume count from ~480 to ~4 for ZDCHcal. Note: layer physVolID is now 0-based (0..N-1) instead of 1-based (1..N), matching the pattern used in other recently-optimised detectors. Also add a comment in ZeroDegreeCalorimeterEcalWSciFi_geo.cpp noting that modVol is already shared and pointing to a potential future paramVolume1D optimisation for the module placement loop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes ZDC sampling geometry construction by sharing repeated layer and slice template volumes instead of creating duplicate TGeoVolume instances for every repetition.
Changes:
- Reworks
ZDC_Samplinglayer construction to create one reusable layer volume per layer group and one reusable slice volume per slice type. - Places repeated layer templates explicitly with distinct layer physVolIDs.
- Adds a clarifying comment to the ScFi ZDC ECAL module placement loop.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/ZeroDegreeCalorimeterSampling_geo.cpp |
Shares layer/slice template volumes across repeated sampling layers. |
src/ZeroDegreeCalorimeterEcalWSciFi_geo.cpp |
Documents that module volume sharing is already used in the 2-D placement loop. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
github-actions
Bot
added
topic: calorimetry
topic: far-forward
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
ZeroDegreeCalorimeterSampling_geo.cpp(used byZDCHcalandZDCEcal) created a new uniquelayer_voland new uniqueslice_volobjects on every iteration of therepeatloop. ForZDCHcal(ZDC_PbScint_NRepeat=120, 3 slices per layer) this inflated the TGeo volume registry with 480 unique TGeoVolumes for entirely identical geometry.This is the same class of inefficiency recently fixed in dRICH, BIC, FEMC, forwardEcal, and BarrelCalorimeterScFi.
Fix
TGeoVolumeper layer group (instead of one per repetition).TGeoVolumeper slice type and place them in the template once.Geometry loads and converts correctly (tested with
geoDisplayagainstZDC_Athena.xml).Notes
ZDCHcalHitscell IDs; no standard ePIC craterlake config currently usesZDC_Athena.xml, so no EICrecon reconstruction is impacted.paramVolume1D(G4PVParameterised) approach was explored but is blocked by two DD4hep 1.37 constraints: (a) the parameterised entity may have at most one daughter, and (b)addPhysVolIDbase must be 0. Since ZDC layers have multiple slices each with independent physVolIDs, shared-template + explicit loop is the correct approach here.ZeroDegreeCalorimeterEcalWSciFi_geo.cpp:modVolwas already shared; added a comment noting the explicit 2-D placement loop and a potential future optimisation.