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

Ecal endcap turbine #347

Merged
merged 14 commits into from
Jul 22, 2024
Merged

Ecal endcap turbine #347

merged 14 commits into from
Jul 22, 2024

Conversation

varnes
Copy link
Contributor

@varnes varnes commented Jul 1, 2024

BEGINRELEASENOTES

  • Added a new driver, ECalEndcap_Turbine_o1_v01, to build a Noble Liquid ECAL endcap with inclined blades (aka turbine geometry)
  • Added a new segmentation (FCCSWEndcapTurbine_k4geo) for the Noble Liquid ECAL endcap turbine geometry
  • Replaced the ALLEGRO_o1_v03 ECAL endcap made of disks perpendicular to the z axis by the turbine geometry built with ECalEndcap_Turbine_o1_v01

ENDRELEASENOTES

This PR is a first pass at including the "turbine" ecal endcap into the G4 simulation, with also a readout segmentation (FCCSWEndcapTurbine_k4geo)

Further details about the turbine geometry and the segmentation used here can be found at https://indico.cern.ch/event/1298458/contributions/5977217/attachments/2875296/5035181/FCC_week_June_2024.pdf

@BrieucF
Copy link
Contributor

BrieucF commented Jul 3, 2024

Hi Erich, thanks for this! Before the actual review, the usual requests:

@varnes
Copy link
Contributor Author

varnes commented Jul 3, 2024

Hi @BrieucF ,

Both README files have now been updated.

Best regards,
Erich

Erich Varnes added 2 commits July 11, 2024 06:31
… reduced, and +z endcap is mirrored to -z rather than begin recreated.
… reduced, and +z endcap is mirrored to -z rather than begin recreated.
@varnes
Copy link
Contributor Author

varnes commented Jul 11, 2024

Hi,

I have committed a new version of the geometry code (following many of the suggestions from Alvaro). This new version is simpler, with the use of boolean shapes reduced. The memory footprint is correspondingly smaller (running ddsim with this geometry adds about 200 MB compared to running with no endcap ECal; the previous version added 1.2 GB). Some bugs appear to have been removed in the process, as I no longer see warnings about volume overlaps, and a material scan through the detector (i.e. in z) shows the expected volume transitions and expected depth in radiation lengths.

@giovannimarchiori
Copy link
Contributor

Hi @BrieucF, as @varnes has updated the READMEs, and the failures in the nightlies are unrelated to this PR (AIDASoft/DD4hep#1292), should we move on with the review?

@BrieucF
Copy link
Contributor

BrieucF commented Jul 18, 2024

@atolosadelgado any further comment?

@atolosadelgado
Copy link
Contributor

atolosadelgado commented Jul 19, 2024

@atolosadelgado any further comment?

geoDisplay and ddsim Geant4 simulation with 100 seems to run. I trust your fixes, I did not check myself again.

I see the following error when trying to visualize,

ERROR: G4VSceneHandler::RequestPrimitives
  Polyhedron not available for layer_shape_0x1ebfce70
  Touchable path:  world_volume_1 0 ECalBarrel_vol_0 0 LAr_bath_5 5 active_107 107 layer_5 5
  This means it cannot be visualized in the usual way on most systems.
  1) The solid may not have implemented the CreatePolyhedron method.
  2) For Boolean solids, the BooleanProcessor, which attempts to create
     the resultant polyhedron, may have failed.

I can come back when I have more information. I do not know if it is important. I guess we can merge and investigate it later, so people doing reconstruction have this subdetector to play with.

@giovannimarchiori
Copy link
Contributor

I don't understand why the error reported by @atolosadelgado mentions ECalBarrel_vol_0, this PR should not affect the barrel at all?
Also, if it's possible to use volumes that exist in ROOT TGeo that'd be much better for visualisation purposes.

@BrieucF
Copy link
Contributor

BrieucF commented Jul 19, 2024

Yes, this error seems not related to this PR, it is very likely there also without the endcap

@BrieucF
Copy link
Contributor

BrieucF commented Jul 19, 2024

Yes, this error seems not related to this PR, it is very likely there also without the endcap

Confirmed, nothing to do with this PR

@atolosadelgado
Copy link
Contributor

Yes, this error seems not related to this PR, it is very likely there also without the endcap

yes, you are right, it is not related to the ECAL endcap. When I remove all other subdetectors except the ECAL barrel, I see the problem. We can debug the ECAL barrel geometry in a different PR. Please, merge this PR if you wish :)

@giovannimarchiori visualization is one of the test I do to check if a geometry is good. ROOT does not check geometries during the visualization, but Geant4 does. If there is an error during the visualization or the overlap check, usually it means there is something wrong with the geometry. If a simulation is done with an ill geometry, it is likely to give strange answers (e.g., the v02 of the drift chamber). I agree ROOT may produce nicer plots of the geometry, but usually that is done once the geometry is confirmed to be good.

@BrieucF BrieucF changed the title E cal endcap turbine2 Ecal endcap turbine Jul 22, 2024
@BrieucF BrieucF merged commit 0bc6a18 into key4hep:main Jul 22, 2024
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants