E cal endcap turbine v3 topo try2 #456

varnes · 2025-03-27T20:58:30Z

To ease the review process, please consider the following before opening a pull request:

the code is sufficiently well documented (e.g. inline comments)
the relevant README(s) were updated. See e.g. https://github.com/key4hep/k4geo/blob/main/detector/calorimeter/README.md or https://github.com/key4hep/k4geo/blob/main/FCCee/IDEA/compact/README.md
the code is covered by tests. See https://github.com/key4hep/k4geo/blob/main/test/CMakeLists.txt
the PR source branch has been rebased (--ff-only) to k4geo/main
the PR does not contain any additions or modifications that do not belong to it
The release notes below contain a succinct and comprehensive description of the changes that are sufficiently self-explanatory

If you are modifying detector dimensions or adding new xml parameters, also consider the following:

the xml file free parameters that can not be modified without additional prescriptions are well indicated
the changes in this PR have not introduced any overlaps. You can check so with the following command: ddsim --compactFile PATH_TO_COMPACT_FILE --runType run --ui.commandsInitialize "/geometry/test/run" > overlapDump.txt

BEGINRELEASENOTES

This PR adds the ability to merge readout cells with the same z and rho indices in adjacent absorber blades. The number of blades to merge in each wheel is controlled by the xml file.

Also, some bugs in calculating the positions of the cells were fixed.

ENDRELEASENOTES

…v3_topo_try2' into ECalEndcap_turbine_v3_topo_try2 I think this is because I used the edit feature on the web page, which put my checked-out version behind

…v3_topo_try2' into ECalEndcap_turbine_v3_topo_try2

atolosadelgado · 2025-04-11T13:03:27Z

Hi @varnes

Thank you for your PR!

Is it ready to be reviewed and merged?

Thank you for your time!

varnes · 2025-04-11T13:32:40Z

Hi @varnes

Thank you for your PR!

Is it ready to be reviewed and merged?

Thank you for your time!

Hi @atolosadelgado ,

Yes, I believe it is ready.

Best regards,
Erich

atolosadelgado · 2025-04-11T13:48:59Z

@varnes

Great! could you please rebase? It seems there will be some conflicts :)

varnes · 2025-04-12T02:01:44Z

Hi @atolosadelgado ,

Just to make sure I don't go down a wrong path (which I find it easy to do in git), is the rebasing needed just to come into sync with commits made by others since I opened this PR? I rebased just before opening it, and also made sure it had minimal changes wrt the main branch (only the pieces I intended to change are different, at least from what I see in the "Files changed" tab).

I did click the "Update branch" button on the PR, which might address some of your concerns? Though I'm admittedly not clear on what exactly that button does.

Thanks,
Erich

atolosadelgado · 2025-04-12T17:03:24Z

Hi @varnes

Scott noticed some overlaps [1], and I was surprise by such big ones, internal to the ECAL endcap, could you please take a look to it?

-------- WWWW ------- G4Exception-START -------- WWWW -------
*** G4Exception : GeomVol1002
      issued by : G4PVPlacement::CheckOverlaps()
Overlap with volume already placed !
          Overlap is detected for volume ECAL_EndCapCryo_front_0:0 (G4Tubs) with ECAL_EndCapCryo_inner_2:2 (G4Tubs)
          overlap at local point (-318.002,199.072,-225) by 2.48263 cm  (max of 15 cases)
--
-------- WWWW ------- G4Exception-START -------- WWWW -------
*** G4Exception : GeomVol1002
      issued by : G4PVPlacement::CheckOverlaps()
Overlap with volume already placed !
          Overlap is detected for volume ECAL_EndCapCryo_back_1:1 (G4Tubs) with ECAL_EndCapCryo_outer_3:3 (G4Tubs)
          overlap at local point (-186.615,-2843.71,200) by 4.98258 cm  (max of 340 cases)
--
-------- WWWW ------- G4Exception-START -------- WWWW -------
*** G4Exception : GeomVol1002
      issued by : G4PVPlacement::CheckOverlaps()
Overlap with volume already placed !
          Overlap is detected for volume ECAL_EndCapCryo_inner_2:2 (G4Tubs) with ECAL_EndCapCryo_front_0:0 (G4Tubs)
          overlap at local point (358.985,119.009,0) by 2.5 cm  (max of 684 cases)
--
-------- WWWW ------- G4Exception-START -------- WWWW -------
*** G4Exception : GeomVol1002
      issued by : G4PVPlacement::CheckOverlaps()
Overlap with volume already placed !
          Overlap is detected for volume ECAL_EndCapCryo_outer_3:3 (G4Tubs) with ECAL_EndCapCryo_front_0:0 (G4Tubs)
          overlap at local point (2604.74,1054.24,0) by 2.5 cm  (max of 1045 cases)
--
-------- WWWW ------- G4Exception-START -------- WWWW -------
*** G4Exception : GeomVol1002
      issued by : G4PVPlacement::CheckOverlaps()
Overlap with volume already placed !
          Overlap is detected for volume LAr_bath_4:4 (G4Tubs) with ECAL_EndCapCryo_front_0:0 (G4Tubs)
          overlap at local point (-760.132,2694.85,0.32899) by 2.4671 cm  (max of 152 cases)

There are more than 500 overlaps up to 0.1mm, maybe worth to document this fact?

[1] #462

…v3_topo_try2' into ECalEndcap_turbine_v3_topo_try2

varnes · 2025-04-18T05:19:52Z

Hi @varnes

Scott noticed some overlaps [1], and I was surprise by such big ones, internal to the ECAL endcap, could you please take a look to it?

-------- WWWW ------- G4Exception-START -------- WWWW -------
*** G4Exception : GeomVol1002
      issued by : G4PVPlacement::CheckOverlaps()
Overlap with volume already placed !
          Overlap is detected for volume ECAL_EndCapCryo_front_0:0 (G4Tubs) with ECAL_EndCapCryo_inner_2:2 (G4Tubs)
          overlap at local point (-318.002,199.072,-225) by 2.48263 cm  (max of 15 cases)
--
-------- WWWW ------- G4Exception-START -------- WWWW -------
*** G4Exception : GeomVol1002
      issued by : G4PVPlacement::CheckOverlaps()
Overlap with volume already placed !
          Overlap is detected for volume ECAL_EndCapCryo_back_1:1 (G4Tubs) with ECAL_EndCapCryo_outer_3:3 (G4Tubs)
          overlap at local point (-186.615,-2843.71,200) by 4.98258 cm  (max of 340 cases)
--
-------- WWWW ------- G4Exception-START -------- WWWW -------
*** G4Exception : GeomVol1002
      issued by : G4PVPlacement::CheckOverlaps()
Overlap with volume already placed !
          Overlap is detected for volume ECAL_EndCapCryo_inner_2:2 (G4Tubs) with ECAL_EndCapCryo_front_0:0 (G4Tubs)
          overlap at local point (358.985,119.009,0) by 2.5 cm  (max of 684 cases)
--
-------- WWWW ------- G4Exception-START -------- WWWW -------
*** G4Exception : GeomVol1002
      issued by : G4PVPlacement::CheckOverlaps()
Overlap with volume already placed !
          Overlap is detected for volume ECAL_EndCapCryo_outer_3:3 (G4Tubs) with ECAL_EndCapCryo_front_0:0 (G4Tubs)
          overlap at local point (2604.74,1054.24,0) by 2.5 cm  (max of 1045 cases)
--
-------- WWWW ------- G4Exception-START -------- WWWW -------
*** G4Exception : GeomVol1002
      issued by : G4PVPlacement::CheckOverlaps()
Overlap with volume already placed !
          Overlap is detected for volume LAr_bath_4:4 (G4Tubs) with ECAL_EndCapCryo_front_0:0 (G4Tubs)
          overlap at local point (-760.132,2694.85,0.32899) by 2.4671 cm  (max of 152 cases)

There are more than 500 overlaps up to 0.1mm, maybe worth to document this fact?

[1] #462

Hi @atolosadelgado ,

Thanks for catching that! These were silly bugs in placing elements of the cryostat, which I've now fixed. There are still many micron-level overlaps that I suspect might take some effort to deal with. So let me know if you'd like to go ahead with the PR as is, or I can put it back into draft status while I work on these overlaps.

Best regards,
Erich

atolosadelgado · 2025-04-22T11:52:53Z

Hi @varnes,

Thanks for fixing the overlaps!

As a first approximation, the missing energy will be proportional to the volume of the overlap. The issue with having many small overlaps is that it introduces instability in the simulation results—each run may expose particles to different amounts of sensitive material.

This problem can be mitigated by including a safety distance, as discussed in other issues in this repo and as implemented in some detectors [1]. Additionally, numerical precision can be affected by nesting certain operations, such as square roots. I noticed your implementation makes extensive use of boolean operations, which may also contribute to these effects.

We can address this either in this PR or in a future one.
@BrieucF @giovannimarchiori, what do you think?

[1]

k4geo/detector/tracker/DriftChamber_o1_v02.cpp

Line 129 in 6f283fa

DCH_length_t safety_r_interspace = 1 * dd4hep::nm;

BrieucF · 2025-04-22T12:22:53Z

Up to @varnes to decide, but I am fine addressing the other overlaps in another PR.

varnes · 2025-04-22T17:31:34Z

Hi @atolosadelgado and @BrieucF ,

I think I have a solution to most of the micron-scale overlaps. There are a few remaining that I don't yet understand where they come from. I'd like a day or so to do some sanity checks to make sure the resolution still looks OK with my changes, and then I'll commit them so the PR can proceed.

Just for the record, I've also noticed in the geometry viewer that there are some small gaps between noble liquid volumes that I also don't understand, but those don't have the same potential for harm that overlaps do, so I'll defer fixing those.

-- Erich

…v3_topo_try2' into ECalEndcap_turbine_v3_topo_try2

varnes · 2025-04-24T13:21:19Z

Hi @atolosadelgado ,

My checks of the energy resolution look fine, so this PR is ready to proceed from my side.

Thanks,
Erich

Erich Varnes and others added 30 commits January 28, 2025 13:05

Add v02 of turbine endcal ecal

5beec4c

Fix up some printouts

3d53b85

Fix printouts

7799539

Fix printouts

19dcfda

Fix printouts

00398c7

Fix printouts

59303dc

Fix printout

62d158f

IDEA with DRC dedicated test added

fea6ea0

fix IDEA with DRC test

98bef5c

apply other SD action to DRC

002c8f4

still not working...

381ce73

replace absolute path by cmake variables in ctest

80b938a

IDEA+DRC test is now working

7d83117

copy TrackerBarrel_o1_v06

b4e2fab

copy CLD_o2_v06 to CLD_o2_v07

a988a5e

update version

0dfacfa

Use new TrackerBarrel

aa1f811

TrackerBarrel_o1_v06 remove using namespace std

8dec7d3

move NeighbourSurfacesData population into a function

33e5dd8

Move LayerLayout population out of the sensor loop

eb3e575

TrackerBarrel_o1_v06 add assembly stave

0fe10d9

addExtensionsToHCalAllegro

69fccc9

implemented suggestions from discussion

bf09fc1

implement comments and improve code readibility

405e0a0

fix failing test and remove unused phi segmentation

036ea5a

fix endcap bitfields and improve readibility

4f675df

keep ALLEGRO v03 xml file unchanged

464ae47

adding version nr also for the barrel xml

e66e3a4

update READMEs

28374e4

add HCalTileBarrel_o1_v02_geo.cpp

a8fc669

Erich Varnes and others added 6 commits March 27, 2025 14:24

Remove unwanted file

ec09954

Add info about module merging

7b9d3a8

Update FCCSWEndcapTurbine_k4geo.cpp

daaca8e

Revise to eliminate large diffs in PR

08d4d02

Merge remote-tracking branch 'refs/remotes/origin/ECalEndcap_turbine_…

a21a912

…v3_topo_try2' into ECalEndcap_turbine_v3_topo_try2 I think this is because I used the edit feature on the web page, which put my checked-out version behind

Revise to reduce size of diff in PR

8485ff2

varnes marked this pull request as ready for review March 28, 2025 03:13

Erich Varnes added 3 commits April 2, 2025 14:10

Fix bug in order of operations that could lead to wrong result

92afdb0

Merge remote-tracking branch 'refs/remotes/origin/ECalEndcap_turbine_…

e623aeb

…v3_topo_try2' into ECalEndcap_turbine_v3_topo_try2

Restore curly brace that was accidentally removed

6d3a07e

Merge branch 'main' into ECalEndcap_turbine_v3_topo_try2

cb553ac

Erich Varnes added 3 commits April 17, 2025 22:10

Fix bugs in placement of cryostat

b6d09c2

Fix bugs in placement of cryostat

8e13134

Merge remote-tracking branch 'refs/remotes/origin/ECalEndcap_turbine_…

07ccfda

…v3_topo_try2' into ECalEndcap_turbine_v3_topo_try2

varnes and others added 3 commits April 22, 2025 10:38

Merge branch 'main' into ECalEndcap_turbine_v3_topo_try2

0a8d7dc

Merge remote-tracking branch 'refs/remotes/origin/ECalEndcap_turbine_…

817a9be

…v3_topo_try2' into ECalEndcap_turbine_v3_topo_try2

Fix some of the um-scale overlap

f7b8ba8

atolosadelgado enabled auto-merge (squash) April 24, 2025 13:25

Merge branch 'main' into ECalEndcap_turbine_v3_topo_try2

e093978

atolosadelgado merged commit 6a4b70a into key4hep:main Apr 24, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

E cal endcap turbine v3 topo try2 #456

E cal endcap turbine v3 topo try2 #456

Uh oh!

varnes commented Mar 27, 2025 •

edited

Loading

Uh oh!

atolosadelgado commented Apr 11, 2025

Uh oh!

varnes commented Apr 11, 2025

Uh oh!

atolosadelgado commented Apr 11, 2025

Uh oh!

varnes commented Apr 12, 2025

Uh oh!

atolosadelgado commented Apr 12, 2025 •

edited

Loading

Uh oh!

varnes commented Apr 18, 2025

Uh oh!

atolosadelgado commented Apr 22, 2025

Uh oh!

BrieucF commented Apr 22, 2025

Uh oh!

varnes commented Apr 22, 2025

Uh oh!

varnes commented Apr 24, 2025

Uh oh!

Uh oh!

Uh oh!

E cal endcap turbine v3 topo try2 #456

E cal endcap turbine v3 topo try2 #456

Uh oh!

Conversation

varnes commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atolosadelgado commented Apr 11, 2025

Uh oh!

varnes commented Apr 11, 2025

Uh oh!

atolosadelgado commented Apr 11, 2025

Uh oh!

varnes commented Apr 12, 2025

Uh oh!

atolosadelgado commented Apr 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varnes commented Apr 18, 2025

Uh oh!

atolosadelgado commented Apr 22, 2025

Uh oh!

BrieucF commented Apr 22, 2025

Uh oh!

varnes commented Apr 22, 2025

Uh oh!

varnes commented Apr 24, 2025

Uh oh!

Uh oh!

Uh oh!

varnes commented Mar 27, 2025 •

edited

Loading

atolosadelgado commented Apr 12, 2025 •

edited

Loading