Migrate IDEA dual-readout calorimeter geometry #292

swkim95 · 2023-09-25T07:52:28Z

BEGINRELEASENOTES

Added DRC geometry construction code under detector/IDEA directory
Added .xml compact files under IDEA directory
Fixed CMakeLists to compile detector/IDEA

ENDRELEASENOTES

Purpose

PR for adding IDEA dual-readout calorimeter (DRC) detector geometry under k4geo package

Tests

Tested compiling k4geo with new code, worked well
Tested running simulation using k4run with 2023-09-18 nightly build key4hep-stack setup, also well worked

…under detector/IDEA

jmcarcell · 2023-09-25T08:33:49Z

Could you edit your post and only put the changes in the release notes? The rest you can put outside. This is how the release notes end up looking like:
https://github.com/key4hep/k4geo/releases/tag/v00-18-01
And there we're only interested in what has been changed and maybe why depending on the change

CMakeLists.txt

BrieucF · 2023-11-15T13:54:01Z

Hi Sungwon, thanks a lot for initiating this! Here are a few general comments:

the detector builder (C++ part) should be under detector/calorimeter. Since you have several files (and that this folder is already busy), maybe it is worth creating a sub-directory detector/calorimeter/dual-readout. What do you think @andresailer ?
can you add a README.md like this one describing briefly the detector builder
Maybe some versioning of the detector builder would be useful as well (_ox_vy in the name)
the detector segmentation should go under detectorSegmentations

SanghyunKo · 2023-12-01T13:31:51Z

Hi @swkim95, would you mind pulling this commit?

…(7-Dec-2023)

Update DRC geometry migration

swkim95 · 2023-12-07T03:56:19Z

Hi,

I updated the PR with latest k4geo & dual-readout changes applied. (7-Dec-2023)
Here are the changes.

I created dual-readout directory under /detector/calorimeter/ and placed the detector builder codes under it.
Moved compact files under /FCCee/IDEA/compact/IDEA_o1_v02/
Removed duplicated DRC compact xml files & detector builder source code under /IDEA/ and /detector/IDEA
Move detector segmentation codes under /detectorSegmentations/ and matched the convention with other files (adding _k4geo)

I haven't added detector builder README.md yet.

SanghyunKo · 2023-12-07T07:26:22Z

detector/calorimeter/dual-readout/src/DRconstructor.cpp

+
+  // verify assumptions
+  if ( std::abs(ymax+ymin) > TGeoShape::Tolerance() )
+    throw std::runtime_error("Envelop of full length fibers (fullBox) is not located at the centre of the tower!");


seems like this commit still hasn't been pulled?

BrieucF

Thanks!

Unfortunately, as such, the detector can not really be integrated in the full IDEA. Building its ROOT geometry alone already takes 14 minutes and 13 GB of RAM. This should be improved, we will provide support (also for the migration to ddsim). Other comments can meanwhile be addressed.

BrieucF · 2024-01-18T15:11:42Z

detector/calorimeter/dual-readout/src/DRgeo.cpp

+    return drDet;
+  }
+} // namespace detector
+DECLARE_DETELEMENT(ddDRcalo, ddDRcalo::create_detector) // factory method


Can you change the name so that it does not clash with https://github.com/HEP-FCC/dual-readout/blob/master/Detector/DRcalo/src/DRgeo.cpp#L74 ? In general we try to have the name of the file matching the name of the factory and we version it. Since we have several dual readout calorimeters, may I suggest to rename the file and the factory to something like FiberDualReadoutCalo_o1_v01?

Hi Brieuc, thanks for the comments!

You mean like DRgeo.cpp -> FiberDualReadoutCalo_o1_v01.cpp and
DECLARE_DETELEMENT(ddDRcalo, ddDRcalo::create_detector) ->
DECLARE_DETELEMENT(FiberDualReadoutCalo_o1_v01, ddDRcalo::create_detector), right?

I'll update like that.

BrieucF · 2024-01-18T15:12:35Z

CMakeLists.txt

@@ -112,7 +116,7 @@ INSTALL(FILES ${hfiles}

 #--- install compact files------------------------------
 if(INSTALL_COMPACT_FILES)
-  INSTALL(DIRECTORY CaloTB CLIC FCalTB FCCee ILD fieldmaps SiD DESTINATION share/k4geo )
+  INSTALL(DIRECTORY CaloTB CLIC FCalTB FCCee ILD fieldmaps SiD IDEA DESTINATION share/k4geo )


This should not be needed anymore

BrieucF · 2024-01-18T15:19:31Z

FCCee/IDEA/compact/IDEA_o1_v02/DRcalo_IDEA.xml

This file should be removed and the detector should be integrated to FCCee/IDEA/compact/IDEA_o1_v02/IDEA_o1_v02.xml. Can you also make sure that the dimensions are compatible with the one here: https://fcc-ee-detector-full-sim.docs.cern.ch/IDEA/ ?

I think it is compatible but not 100% sure...
I'll ask Sanghyun about it.

About various DRcalo xml files, I'm also not sure which one to use.
I'll also discuss this with Sanghyun and fix which xml file to include.

BrieucF · 2024-01-18T15:24:02Z

FCCee/IDEA/compact/IDEA_o1_v02/DRcalo_Wratten9_S13615-1025.xml

This file should be split:

global "envelope parameters" should go to FCCee/IDEA/compact/IDEA_o1_v02/DectDimensions_IDEA_o1_v01.xml (please profit from this occasion to remove all the other ECAL/HCAL dimensions which will no longer be used in this file)

detector specific parameters should go to a dedicated file that I propose to name FCCee/IDEA/compact/IDEA_o1_v02/FiberDualReadoutCalo_o1_v01.xml

missing materials/elements should go to FCCee/IDEA/compact/IDEA_o1_v02/elements_o1_v01.xml and FCCee/IDEA/compact/IDEA_o1_v02/materials_o1_v01.xml

BrieucF · 2024-01-18T15:34:29Z

FCCee/IDEA/compact/IDEA_o1_v02/DRcalo.xml

Let's try to keep one file for the detector description (see the other comment on FCCee/IDEA/compact/IDEA_o1_v02/DRcalo_Wratten9_S13615-1025.xml)

Update k4geo to latest commit & split detector xml files

…) in CMakeList compact file installation part

…RC detector constructors, plugins for running simluation with IDEA DRC using ddsim

PR update with latest k4geo master branch & adding IDEA DRC simulation

…ies in materials_o1_v01.xml and move them to DRC xml file), remove duplicated DRC geo constructor (DRgeo.cpp), update DRconstructor.cpp to remove commented out codes etc..., remove scripts for debug in plugin

SanghyunKo

@swkim95 there are some places still need to be cleaned up

SanghyunKo · 2024-06-03T07:19:03Z

FCCee/IDEA/compact/IDEA_o1_v03/DectDimensions_IDEA_o1_v03.xml

+    <constant name="FiberDRCalo_barrel_inner_radius" value="2.5*m"/>
+    <constant name="FiberDRCalo_barrel_nphi"         value="144"/>
+    <constant name="FiberDRCalo_endcap_height"       value="2.*m"/>
+    <constant name="FiberDRCalo_endcap_inner_radius" value="2.5*m"/>


Would you mind changing FiberDRCalo_endcap_inner_radius to FiberDRCalo_endcap_z_start? (as discussed in the mattermost)

SanghyunKo · 2024-06-03T07:22:26Z

detector/calorimeter/dual-readout/include/DRconstructor.h

+
+  private:
+    void implementTowers(xml_comp_t& x_theta, dd4hep::DDSegmentation::DRparamBase_k4geo* param);
+    // void placeAssembly(xml_comp_t& x_theta, xml_comp_t& x_wafer, dd4hep::DDSegmentation::DRparamBase_k4geo* param,


It'd be better to clean up comments (in other places as well)

SanghyunKo · 2024-06-03T07:23:27Z

detector/calorimeter/dual-readout/src/FiberDualReadoutCalo_o1_v01.cpp

+namespace ddDRcalo {
+  static dd4hep::Ref_t create_detector( dd4hep::Detector &description, xml_h xmlElement, dd4hep::SensitiveDetector sensDet ) {    
+    // For Debug
+    // std::cout << __PRETTY_FUNCTION__  << std::endl;


same as above

SanghyunKo · 2024-06-03T07:25:40Z

FCCee/IDEA/compact/IDEA_o1_v02/IDEA_o1_v02.xml

@@ -52,6 +52,9 @@

  <!-- Import Endcap plate absorber -->
  <include ref="EndPlateAbsorber_o1_v01.xml"/>
+
+  <!-- Import fiber-based dual-readout calorimeter -->
+  <include ref="FiberDualReadoutCalo_o1_v01.xml"/>


I think we only need to push to the v03, not v02 (as we discussed in the working meeting)

SanghyunKo · 2024-06-03T07:27:01Z

plugins/DRCaloFastSimModel.cpp

Please fix indents

SanghyunKo · 2024-06-03T07:27:37Z

plugins/FiberDRCaloSDAction.cpp

Same as above (indents)

SanghyunKo · 2024-06-03T07:27:58Z

plugins/FiberDRCaloSDAction.h

Same as above (indents)

swkim95 · 2024-06-05T07:27:04Z

Hi, I just updated the codes based on Sanghyun's comments

SanghyunKo · 2024-06-05T09:36:06Z

FCCee/IDEA/compact/IDEA_o1_v02/IDEA_o1_v02.xml

@@ -52,7 +52,7 @@

  <!-- Import Endcap plate absorber -->
  <include ref="EndPlateAbsorber_o1_v01.xml"/>
-
+  


Not a major comment but it's a good practice to revert your changes using git checkout -- <filename> or equivalent instead of removing texts by hands... (so that you don't screw up commit histories)

swkim95 added 2 commits September 21, 2023 17:20

with latest dual-readout (2023-09-21) pacakge, migrated DRC geometry …

f45e63c

…under detector/IDEA

Remove cout used for debugging

bd28ad7

andresailer requested a review from BrieucF October 3, 2023 13:49

andresailer reviewed Oct 31, 2023

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

andresailer marked this pull request as draft November 1, 2023 14:10

swkim95 and others added 5 commits December 7, 2023 12:28

DRC migration code update with latest dual-readout and k4geo package …

5c9075a

…(7-Dec-2023)

Merge branch 'DRC_geo_migrate_230918' into DRC_geo_migrate_231113

4c8a5be

Merge pull request #1 from swkim95/DRC_geo_migrate_231113

5280557

Update DRC geometry migration

remove cout for the debug

ccabc2f

Remove duplicated DRC compact file & source codes under IDEA directory

9d25eb5

SanghyunKo suggested changes Dec 7, 2023

View reviewed changes

swkim95 and others added 3 commits December 8, 2023 00:28

Update DRC detetor builder to latest version

4ea51dc

Merge branch 'main' into DRC_geo_migrate_230918

4ebaae3

Merge branch 'key4hep:main' into DRC_geo_migrate_230918

b0bfcf9

BrieucF requested changes Jan 18, 2024

View reviewed changes

swkim95 and others added 11 commits March 7, 2024 16:21

Update branch with latest k4geo & Split FiberDRC xml files

0a96366

remove debugging cout

df5d782

Merge branch 'DRC_geo_migrate_230918' into PRupdate

d09dc81

Merge pull request #2 from swkim95/PRupdate

369d1ec

Update k4geo to latest commit & split detector xml files

Remove unused xml files, remove IDEA directory (which does not exists…

ccd166e

…) in CMakeList compact file installation part

Updates in IDEA XML files for implemeting Dual-readout calorimeter, D…

86941a0

…RC detector constructors, plugins for running simluation with IDEA DRC using ddsim

Merge branch 'DRC_geo_migrate_230918' into PRupdate_240408

ccd8dfc

Merge pull request #3 from swkim95/PRupdate_240408

ad54c67

PR update with latest k4geo master branch & adding IDEA DRC simulation

Fix xml files (remove redundant includes, remove DRC material propert…

2c222d2

…ies in materials_o1_v01.xml and move them to DRC xml file), remove duplicated DRC geo constructor (DRgeo.cpp), update DRconstructor.cpp to remove commented out codes etc..., remove scripts for debug in plugin

Update branch to latest k4geo master branch

f5ed2f2

Fix units in SD action plugin, add wavelength information in output file

09107f1

swkim95 added 2 commits June 1, 2024 16:29

Add Fiber DRC xml to IDEA v03

dccbd0a

remove debugging message, find EDM4HEP for plugins

9674154

SanghyunKo suggested changes Jun 3, 2024

View reviewed changes

swkim95 marked this pull request as ready for review June 3, 2024 08:41

swkim95 added 2 commits June 5, 2024 16:07

Remove DRC from IDEA v02

806a6af

Fix indentation

1a6c652

SanghyunKo reviewed Jun 5, 2024

View reviewed changes

BrieucF assigned atolosadelgado Jun 25, 2024

swkim95 closed this Jun 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate IDEA dual-readout calorimeter geometry #292

Migrate IDEA dual-readout calorimeter geometry #292

swkim95 commented Sep 25, 2023 •

edited

Loading

jmcarcell commented Sep 25, 2023

BrieucF commented Nov 15, 2023

SanghyunKo commented Dec 1, 2023

swkim95 commented Dec 7, 2023

SanghyunKo Dec 7, 2023

BrieucF left a comment

BrieucF Jan 18, 2024

swkim95 Jan 24, 2024

BrieucF Jan 24, 2024

BrieucF Jan 18, 2024

BrieucF Jan 18, 2024

swkim95 Jan 24, 2024

BrieucF Jan 18, 2024

BrieucF Jan 18, 2024

SanghyunKo left a comment

SanghyunKo Jun 3, 2024

SanghyunKo Jun 3, 2024

SanghyunKo Jun 3, 2024

SanghyunKo Jun 3, 2024

SanghyunKo Jun 3, 2024

SanghyunKo Jun 3, 2024

SanghyunKo Jun 3, 2024

swkim95 commented Jun 5, 2024

SanghyunKo Jun 5, 2024

		@@ -52,7 +52,7 @@

		<!-- Import Endcap plate absorber -->
		<include ref="EndPlateAbsorber_o1_v01.xml"/>

Migrate IDEA dual-readout calorimeter geometry #292

Migrate IDEA dual-readout calorimeter geometry #292

Conversation

swkim95 commented Sep 25, 2023 • edited Loading

jmcarcell commented Sep 25, 2023

BrieucF commented Nov 15, 2023

SanghyunKo commented Dec 1, 2023

swkim95 commented Dec 7, 2023

Choose a reason for hiding this comment

BrieucF left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SanghyunKo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swkim95 commented Jun 5, 2024

Choose a reason for hiding this comment

swkim95 commented Sep 25, 2023 •

edited

Loading