feat: switch BIC, FEMC, and EcalBarrelScFi to paramVolume1D by wdconinc · Pull Request #1051 · eic/epic

wdconinc · 2026-02-26T19:00:18Z

Briefly, what does this PR introduce?

This PR converts loops of explicit placement to parametrized placement in the BIC, FEMC, and EcalBarrelScFi detectors. This reduces the TGeoNode count, speeds up geometry initialization, and reduces memory use.

For BIC and FEMC: modules/towers are placed via paramVolume1D.

For EcalBarrelScFi: scintillating fibers are placed via paramVolume1D, one call per HCP z-row per readout grid. Each z-row is wrapped in a thin Assembly with physVolID("z", row_index) to give unique VolumeManager cell-ID codes across rows (required because DD4hep parameterised volumes can only carry volID=0; the Geant4 copy number encodes the column index into the "fiber" readout field at runtime).

Readout change for EcalBarrelScFi: the z field now encodes the fiber row index within the grid (previously always 0), and fiber encodes the column index within that row (previously a sequential per-grid ID). The total set of unique cell IDs is unchanged; the encoding has changed.

Also fixes an O(N_fibers × N_grids) performance issue in geometry initialization: the TGeoPolygon for each grid is now built once instead of once per fiber.

What kind of change does this PR introduce?

Bug fix (issue #__)
New feature (issue #__)
Documentation update
Other: __

Please check if this PR fulfills the following:

Tests for the changes have been added
Documentation has been added / updated
Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

EcalBarrelScFi readout: the z and fiber fields have new semantics (row index and column index within row, respectively). Reconstruction code that uses these fields for individual fiber identification needs updating.

Does this PR change default behavior?

Yes — EcalBarrelScFi readout IDs change encoding.

wdconinc

For BIC, i_module is now identical to i_y.

github-actions · 2026-04-30T03:23:52Z

Capybara summary for PR 1051

wdconinc · 2026-05-26T23:28:20Z

I think copilot has successfully convinced me that this geometry change modifies stepping in such a way that we don't expect the same hits for tracks that go through the modified geometry since the navigator behaves differently for parametrized and non-parametrized volumes.

Copilot

Pull request overview

This PR updates the DD4hep geometry construction for the Forward ECal (FEMC) and Barrel Imaging Calorimeter (BIC) to use paramVolume1D (Geant4 parameterised placement) instead of explicit placement loops, with the goal of reducing the number of G4PVPlacement instances and improving Geant4 navigation performance in these subdetectors.

Changes:

Replace per-column block placement loops in FEMC with a 1D parameterised placement per row.
Replace per-module placement loops in BIC’s xy_layout with a 1D parameterised placement (requiring nx == 1) and reconstruct per-copy DetElement placements from the parameterisation’s placement list.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/forwardEcal_geo.cpp	Switch FEMC block column placements to `paramVolume1D` using an `Assembly` template to reduce placements per row.
src/BarrelCalorimeterImaging_geo.cpp	Switch BIC module placements in `xy_layout` to `paramVolume1D` (1D along Y), and rebuild per-module `DetElement` placements from the parameterised placement list.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

wdconinc · 2026-05-28T20:46:35Z

EcalBarrelScFi z encoding is wrong here.

akioogawa · 2026-05-29T18:14:09Z

                   slice_thickness / 2.0);
          Volume frow_vol("FEMCFiberRow", frow, Wpowder);
          frow_vol.setAttributes(desc, x_slice.regionStr(), x_slice.limitsStr(), x_slice.visStr());
          for (int iy = 0; iy < ny; iy++) {


Can this loop be also using paramVolume1D?

No, there is staggering of the layers inside it. That makes it harder to do. It may (@veprbl claims) be possible to do this with two paramVolume1D instances (one for even, one for odd), or it is may be possible to do this with a 2-fiber unit cell, but in any case it is more complicated than just a parametrized placement. And it would likely change the numbering scheme of the fibers. And it would get is proportionally less gain than this part gets us since we are now starting to bump up against other irreducible effects.

No, there is staggering of the layers inside it.

I guess what I meant here is that you can't easily use paramVolume2D.

with two paramVolume1D

with one paramVolume3D

Ok, I'll need a picture for this...

akioogawa · 2026-05-29T18:14:45Z

        //rows of blocks
        //int nRowBlock = map->maxRowBlock(); //# of rows
        int nRowBlock = mMaxRowBlock;
        for (int r = 0; r < nRowBlock; r++) {


And this loop?

akioogawa · 2026-05-29T18:15:56Z

Sorry I did not realize this was up. Now I'm checking out and compiling. I will be testing this (maybe next week).

Co-authored-by: Wouter Deconinck <wdconinc@gmail.com>

for more information, see https://pre-commit.ci

Replace explicit placeVolume per fiber with paramVolume1D (one call per z-row per grid), reducing TGeoNode count from O(N_fibers) to O(N_rows). Each z-row of fibers is wrapped in a row Assembly with physVolID("z", row_index), which provides unique VolumeManager cell-ID codes across rows. Within each row, paramVolume1D encodes the fiber column index via the Geant4 copy number into the "fiber" readout field. Readout semantics change: the "z" field now encodes the fiber row index within the grid (previously always 0), and "fiber" encodes the column index within that row (previously a sequential global-per-grid ID). Also fixes an O(N_fibers × N_grids) performance issue in geometry initialization: the TGeoPolygon for each grid is now built once, not once per fiber checked against that grid. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…0 for segmentation The 'z' field in EcalBarrelScFiHits readout is used by CartesianStripZ segmentation (identifier_x="z") to encode the longitudinal position along the fiber axis. The physVolID z must therefore remain 0 from the geometry side. Replace the previous approach of addPhysVolID("z", row_index) on the row Assembly with addPhysVolID("fiber", row_index * n_x_max), where n_x_max is the maximum row length rounded up to the next power of two. DD4hep ORs all physVolID values along the path into the cellID code: - init: fiber = row_index * n_x_max | 0 = row_index * n_x_max - runtime: fiber = row_index * n_x_max | copy_no = row_index * n_x_max + copy_no This encodes (row, column) into the existing 16-bit fiber field with no bit overlap, preserving uniqueness within each grid and leaving the z field entirely free for the longitudinal segmentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Replace three explicit placement loops inside the fEcalWScFi ScFi block hierarchy with DD4hep paramVolume1D placements: - Tower rows (trow) in block: 4 → 1 paramVolume1D (uniform Y spacing) - Towers in tower row: 4 → 1 paramVolume1D (uniform X spacing) - Fiber columns (fcol) in fiber row: 26 → 1 paramVolume1D (uniform X spacing) The staggered fiber row (frow) loop (ny=30) is kept as explicit placements since alternating x-offset (0 / +½ pitch) prevents simple 1D parameterisation. Net effect: ~400× reduction in G4PVPlacement count per block (from ~12,480 to ~30 per block × ~1073 blocks). The block-level paramVolume1D (already present) is unchanged. physVolID encoding is preserved: copy number equals the original loop index for y (0-3), x (0-3), and fiber_x (0-25). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

akioogawa · 2026-06-18T19:08:34Z

something is wrong... https://www.star.bnl.gov/public/spin/akio/epic/map.homo.png. Nope that was my bug in plotting. Geometry looks good in dd_web_display for both Homogeneous and SiPM. Hit mapping seems to be same as before. dE for gamma looks good.

akioogawa

Looks good

github-actions Bot added topic: barrel Mid-rapidity detectors topic: calorimetry labels Feb 26, 2026

wdconinc force-pushed the parametrized-volumes branch from af6aa53 to 3bcf1e6 Compare April 29, 2026 21:49

wdconinc commented Apr 29, 2026

View reviewed changes

Comment thread src/BarrelCalorimeterImaging_geo.cpp Outdated

Comment thread src/BarrelCalorimeterImaging_geo.cpp Outdated

Comment thread src/BarrelCalorimeterImaging_geo.cpp Outdated

Comment thread src/BarrelCalorimeterImaging_geo.cpp Outdated

pre-commit-ci Bot temporarily deployed to github-pages April 30, 2026 03:21 Inactive

wdconinc force-pushed the parametrized-volumes branch from 6757f31 to 5b3378d Compare May 23, 2026 22:59

wdconinc temporarily deployed to github-pages May 23, 2026 23:57 — with GitHub Actions Inactive

wdconinc temporarily deployed to github-pages May 25, 2026 23:38 — with GitHub Actions Inactive

wdconinc force-pushed the parametrized-volumes branch from 5b3378d to efd95d0 Compare May 26, 2026 03:34

wdconinc temporarily deployed to github-pages May 26, 2026 04:25 — with GitHub Actions Inactive

wdconinc force-pushed the parametrized-volumes branch from efd95d0 to ad30ff6 Compare May 26, 2026 20:25

wdconinc temporarily deployed to github-pages May 26, 2026 23:02 — with GitHub Actions Inactive

wdconinc marked this pull request as ready for review May 26, 2026 23:28

Copilot AI review requested due to automatic review settings May 26, 2026 23:28

Copilot started reviewing on behalf of wdconinc May 26, 2026 23:28 View session

Copilot AI reviewed May 26, 2026

View reviewed changes

wdconinc requested review from a team, akioogawa, mariakzurek and veprbl May 27, 2026 00:13

wdconinc changed the title ~~feat: switch BIC and FEMC to paramVolume1D~~ feat: switch BIC, FEMC, and EcalBarrelScFi to paramVolume1D May 28, 2026

wdconinc temporarily deployed to github-pages May 28, 2026 20:42 — with GitHub Actions Inactive

wdconinc marked this pull request as draft May 28, 2026 20:46

wdconinc temporarily deployed to github-pages May 28, 2026 22:23 — with GitHub Actions Inactive

wdconinc temporarily deployed to github-pages May 29, 2026 00:51 — with GitHub Actions Inactive

wdconinc mentioned this pull request May 29, 2026

DRICH: move PDU placements into per-sector sensorbox subvolumes #1115

Open

akioogawa reviewed May 29, 2026

View reviewed changes

akioogawa temporarily deployed to github-pages May 29, 2026 19:39 — with GitHub Actions Inactive

wdconinc added this to ePIC Review Board (auto-populated; changes will be discarded) May 30, 2026

wdconinc moved this to In Review in ePIC Review Board (auto-populated; changes will be discarded) May 30, 2026

wdconinc and others added 6 commits June 6, 2026 16:17

feat: switch BIC and FEMC to paramVolume1D

aa61ae5

fix: rm unused i_module in BIC

e1d8591

Co-authored-by: Wouter Deconinck <wdconinc@gmail.com>

[pre-commit.ci] auto fixes from pre-commit.com hooks

2422621

for more information, see https://pre-commit.ci

wdconinc force-pushed the parametrized-volumes branch from bb7123c to 5c941de Compare June 6, 2026 21:17

wdconinc temporarily deployed to github-pages June 6, 2026 23:19 — with GitHub Actions Inactive

akioogawa approved these changes Jun 19, 2026

View reviewed changes

wdconinc moved this from In Review to Approved in ePIC Review Board (auto-populated; changes will be discarded) Jun 19, 2026

Uh oh!

Conversation

wdconinc commented Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Briefly, what does this PR introduce?

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

Does this PR introduce breaking changes? What changes might users need to make to their code?

Does this PR change default behavior?

Uh oh!

wdconinc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Capybara summary for PR 1051

Uh oh!

wdconinc commented May 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

wdconinc commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akioogawa May 29, 2026

Choose a reason for hiding this comment

Uh oh!

wdconinc May 29, 2026

Choose a reason for hiding this comment

Uh oh!

wdconinc May 29, 2026

Choose a reason for hiding this comment

Uh oh!

veprbl May 29, 2026

Choose a reason for hiding this comment

Uh oh!

wdconinc May 29, 2026

Choose a reason for hiding this comment

Uh oh!

akioogawa May 29, 2026

Choose a reason for hiding this comment

Uh oh!

akioogawa commented May 29, 2026

Uh oh!

akioogawa commented Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akioogawa left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wdconinc commented Feb 26, 2026 •

edited

Loading

github-actions Bot commented Apr 30, 2026 •

edited

Loading

wdconinc commented May 28, 2026 •

edited

Loading

akioogawa commented Jun 18, 2026 •

edited

Loading