eic / epic Goto Github PK

Is your feature request related to a problem? Please describe.
It would be useful to be notified if certain subsystem parameters are changed by a pull request. Some changes in one subsystem can have unintended side effects in others, and these side effects can easily slip under the radar.

Describe the solution you'd like
A quick way is to diff the constants.out artifact between the current PR and the one from the last push to main. That's probably too much information though, and it would be better to have a much smaller list of specific parameters that we want to watch for changes on.

The idea is to first produce a "lock" file adding certain parameters and their required/preferred values; likely this would be similar to the Menagerie. The parameters in this lock file are the ones we want to keep fixed, or at least be notified when they are changed. For example, the dRICH envelope:

DRICH_rmax2 180.0
DRICH_zmin  195.0
DRICH_zmax  315.0

We should aim to keep this list as minimal as possible.

Then add a CI job that would check if any of the parameters in this lock file differ from the implemented parameter values in the associated pull request. If there are changes, warn the user and fail the job.

Changes to the lock file should be allowed by anyone, with the approval of relevant subsystems (liaisons?). Changes shouldn't be restricted by any automated link to the menagerie, but it would be good to compare the lock file to the menagerie regularly.

Describe alternatives you've considered
Hard code such a check in a src/*.cpp file, and printout a warning.

The EPIC barrel HCAL needs to be updated to match the sPHENIX oHCAL geometry. The "as-built" geometry is available in a set of gdml files that are used directly in G4 in the sPHENIX Fun4All. The nice part about the way this is implemented is that the absorber structures are defined separately (in separate gdml files) from the sensitive elements (the tiles). I think this means it should be possible to handle the fact that gdml doesn't include the ability to flag sensitive elements and wrap the gdml in an xml that dd4hep will be happy with.

Based on the tutorials over the past two weeks I have started some work on this.

Is your feature request related to a problem? Please describe.
Users may make the epic code and end up with an epic.xml file in their source dir. They may do something with this file, which refers to environment variables. That may lead to confusing results.

Describe the solution you'd like
With produced xml files in the build directory, it is less likely that they would be used inadvertently assuming they work from the source dir top level.

Describe alternatives you've considered
Alternative is not changing.

• branch - main
• Fresh ubuntu 22.04

When building the detector:

/usr/bin/env: /usr/bin/env: ‘python’‘python’: No such file or directory: No such file or directory

While there are solutions like:

sudo apt install python-is-python3

I would advocate to fix it in epic repo by switching to python3. It is now everywhere

/usr/bin/env python3

Reducing tolerance to 0.01 mm causes a LOT of SciGlassModule overlaps to be detected

Originally posted by @c-dilks in #26 (comment)

Is your feature request related to a problem? Please describe.
Currently, our simulations read in the 1.5 T fieldmap from sPhenix.2d.dat. This map is not rescaled to get the planned 1.7 T magnetic field.

Describe the solution you'd like
We should get input from GD/I and/or tracking on what the fieldmap configuration should be in the short term.

-------- WWWW ------- G4Exception-START -------- WWWW -------
*** G4Exception : GeomVol1002
      issued by : G4PVPlacement::CheckOverlaps()
Overlap with volume already placed !
          Overlap is detected for volume AV_586!InnerMPGDBarrel_Mod1_88#88!component1_1#1:90 (G4Box)
          apparently fully encapsulating volume AV_585!InnerMPGDBarrel_Mod1_87#87!component0_0#0:88 (G4Box) at the same level!
NOTE: Reached maximum fixed number -1- of overlaps reports for this volume !
*** This is just a warning message. ***
-------- WWWW -------- G4Exception-END --------- WWWW -------

Originally posted by @veprbl in #107 (comment)

Is your feature request related to a problem? Please describe.
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

Describe the solution you'd like
A clear and concise description of what you want to happen.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

Describe the solution you'd like
We currently have acceptance losses along sector boundaries at high theta. It seems we have enough space for more sensors to cover this region, so let's add some and see if we can improve.

Environment: (where does this bug occur, have you tried other environments)

• Which branch (main for latest released):
• Which revision (HEAD for the most recent):
• Any specific OS or system where the issue occurs?
• Any special versions of ROOT or Geant4?

Steps to reproduce: (give a step by step account of how to trigger the bug)

1. Originally with ATHENA bHCAL
2. Set segmentation to GridPhiEta
3. Run simulation

Expected Result: (what do you expect when you execute the steps above)

hit distributions in phi,eta

Actual Result: (what do you get when you execute the steps above)

all hits at eta=0, segmentation fails

Probably still an issue for EPIC

Is your feature request related to a problem? Please describe.
With the transition to GitHub we have lost the ability to direct load root files into geoviewer. That was a compelling feature that we wish to retain.

Describe the solution you'd like
We should write up-to-date geometry root files as output of a repository main branch pipeline on eicweb.

Describe alternatives you've considered
We can have the GitHub pipelines deploy to ... somewhere. But it is not clear where that would be.

Is your feature request related to a problem? Please describe.
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

Describe the solution you'd like
A clear and concise description of what you want to happen.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

Is your feature request related to a problem? Please describe.
The number of staves is different between the barrel calorimeter and DIRC, better to match them.
Now the hits type "calorimeter hit" will merge all hits in a single segment, and thus one single fiber only produces one signal in an event. In reality, we could have a waveform readout and separate the hits if they were distant enough (deconvolution of signals in the time domain).

Describe the solution you'd like
The implementation should be simply done by changing the compact (xml) files.
Number of staves: change one parameter ("CaloSides") in definitions.xml
Hits separation for fibers: add z-segmentation for fibers with a size smaller than the realistic resolution (~1cm), so we can produce the waveform signals with additional z information in digitization.

Describe alternatives you've considered
Hits separation for fibers: Change the sensitive volume type to "tracker" but this would result in a huge amount of hits registered in the simulation output.

Inner barrel MPGD detector is currently a continuous detector. In reality this detector will need to be segmented into tiles based on raw material constraints, and support materials need to be added

Is your feature request related to a problem? Please describe.
The outer MPGD layer behind the DIRC needs to be changed from cylindrical geometry into rectangular geometry that mimics the geometry of the DIRC bars.

Describe the solution you'd like
Replace the cylindrical outer MPGD layer with rectangular geometry.

Additional context
This layer is integrated into the DIRC geometry and will sit in a 2cm space just above the DIRC bars

Is your feature request related to a problem? Please describe.
MPGD does not fit since 'fake' DIRC is not implemented correctly (uses envelope).

Describe the solution you'd like
Disable fake_dirc.xml entirely, and only merge DIRC back in when it is a real working DIRC.

Describe alternatives you've considered
Fix the 'fake' DIRC. Not useful to spend effort there.

Additional context
@mposik1983

Environment: (where does this bug occur, have you tried other environments)

main branch, likely introduced from #20

Steps to reproduce: (give a step by step account of how to trigger the bug)

Evident in geoviewer and dawn artifacts

Expected Result: (what do you expect when you execute the steps above)

DRICH_zmin                     =      195.000
DRICH_zmax                     =      315.000

Actual Result: (what do you get when you execute the steps above)

DRICH_zmin                     =      208.000
DRICH_zmax                     =      328.000

Is your feature request related to a problem? Please describe.
Check to make sure if this is what we really want:

Describe the solution you'd like
We only use sector and module downstream in the current reconstruction code, but we should use x and y too. Any changes here in epic need to be reflected (and validated) in https://github.com/eic/drich-dev/blob/main/src/draw_segmentation.cpp

For x and y, ideally we want 0-7, unsigned, and (maybe?) the highest 32 bits.

Describe alternatives you've considered
Ignoring the segmentation in reconstruction, and doing our own (which is what we are currently doing...)

Additional context
Be careful of off-by-one errors. In ATHENA, we actually were running with a segmentation of 9x9, thinking it was 8x8 (but it didn't matter, since again we did our own segmentation in the reconstruction code). This off-by-one issue is the reason for the -1 in the denominator of grid_size_x,y

Describe the solution you'd like
Implement dual or multi mirrors (per sector) in the dRICH. This will allow us to place sensors in a more confined area.

For a first attempt at this in DD4hep (athena), see https://eicweb.phy.anl.gov/EIC/detectors/athena/-/merge_requests/260

Is your feature request related to a problem? Please describe.
Right now there is virtually no checking performed on this repository.

Describe the solution you'd like
For both branches and potentially forks, this repository should trigger a CI pipeline on eicweb which will pull the necessary code from the originating branch (in case of a pull request from a fork).

Is your feature request related to a problem? Please describe.
The silicon tracker disks at the expanding incoming and outgoing beampipe must have a hole that is off-centered.

Describe the solution you'd like
In the xml file we should add parameters for the offset, e.g. TrackerEndcapNLayer4_offsetx, and pass them to the envelope, e.g.,

        <envelope vis="TrackerLayerVis"
          rmin="TrackerEndcapNLayer4_rmin"
          rmax="TrackerEndcapNLayer4_rmax"
          offsetx="TrackerEndcapNLayer4_offsetx"
          length="SiTrackerEndcapLayer_thickness"
          zstart="TrackerEndcapNLayer4_zmin" />

and we would need to add support to the src/TrapEndcapTracker_geo.cpp, such as:

    std::string layer_vis      = l_env.attr<std::string>(_Unicode(vis));
    double      layer_rmin     = l_env.attr<double>(_Unicode(rmin));
    double      layer_rmax     = l_env.attr<double>(_Unicode(rmax));
    double      layer_offsetx  = l_env.attr<double>(_Unicode(offsetx));
    double      layer_length   = l_env.attr<double>(_Unicode(length));
    double      layer_zstart   = l_env.attr<double>(_Unicode(zstart));
    double      layer_center_z = layer_zstart + layer_length / 2.0;
// ...
    Tube   layer_tub_outer(0, layer_rmax, layer_length / 2);
    Tube   layer_tub_inner(0, layer_rmin, layer_length / 2);
    SubtractionSolid layer_tub(layer_tub_outer, layer_tub_inner, Translation3D(layer_offsetx, 0, 0));

Describe alternatives you've considered
Keep larger central hole, but that cuts into acceptance.

Additional context
There will additional work needed to then place the rings correctly. Maybe we need to add a phi range for the rings, or otherwise allow for incomplete rings.

Is your feature request related to a problem? Please describe.
Rescale the pfRICH for the BryceCanyon configuration (see #126)

Describe the solution you'd like

Rescaled envelope parameters (TBD):

Length ~44 cm
Proximity gap 30 cm
Services (LAPPD) 10 cm
Z position (end): -172.7 cm

See Elke's slides for more details

For reference, the parameters from ATHENA:

PFRICH_Length                  =       58.000 = BackwardRICHRegion_length
PFRICH_aerogel_thickness       =        3.000 = 3.0*cm
PFRICH_num_px                  =        8.000 = 8
PFRICH_rmax                    =       93.000 = BackwardPIDRegion_rmax - 2*cm
PFRICH_rmin0                   =        5.006 = BackwardPIDRegion_tan * BackwardRICHRegion_zmin
PFRICH_rmin1                   =        6.942 = BackwardPIDRegion_tan * (BackwardRICHRegion_zmin + BackwardRICHRegion_length)
PFRICH_sensor_active_size      =        2.400 = PFRICH_sensor_active_size_default
PFRICH_sensor_dist             =       40.000 = 40*cm
PFRICH_sensor_full_size        =        2.580 = PFRICH_sensor_full_size_default
PFRICH_sensor_thickness        =        0.150 = 1.5*mm
PFRICH_wall_thickness          =        0.500 = 0.5*cm
PFRICH_window_thickness        =        0.100 = 0.1*cm
PFRICH_zmax                    =     -208.000 = PFRICH_zmin - PFRICH_Length
PFRICH_zmin                    =     -150.000 = -BackwardRICHRegion_zmin

Is your feature request related to a problem? Please describe.
The IRT auxiliary ROOT file contains IRT geometry objects, used to configure the IRT code. See https://eicweb.phy.anl.gov/EIC/detectors/ecce/-/merge_requests/31 for details.

We currently use it as a temporary "backdoor" to the geometry, until we can establish a geometry service usage within the new reconstruction framework. To enable auxiliary file creation:

• cmake with -DIRT_AUXFILE=ON
• set drich.xml constant DRICH_create_irt_file to 1
• construct the detector, for example

kernel = DDG4.Kernel()
kernel.loadGeometry('file:ecce.xml')
kernel.terminate()

This is not a sustainable implementation, however. When geometry services are available in the new reconstruction framework, and we have tested that the same IRT objects can be created from there, we plan to remove this backdoor completely.

Describe the solution you'd like

• cross check with IRT geometry objects created from JANA2
• remove all if(DEFINED IRT_AUXFILE) blocks from CMakeLists.txt
• remove all #ifdef IRT_AUXFILE blocks from src/DRICH_geo.cpp
• remove DRICH_create_irt_file and irt_filename from compact/drich.xml

Adding TOF in DD4HEP. The details are described in the following presentations

Barrel TOF (Zhenyu): https://indico.bnl.gov/event/16765/
The CTTL consists of one AC-LGAD layer, with 500um x 1cm strips along the beam direction. It provides <30um spatial resolution in φ direction, and <25 ps timing resolution per hit. The CTTL is located at R ~ 0.625-0.655 m and has a total length of 2.7 m from z~ -1.200 to 1.500 m.

Endcap (Nicolas): https://indico.bnl.gov/event/16467/
Electron Timing and Tracking Layer (ETTL)
The ETTL consists of one AC-LGAD layer, with 500um x 500 um pixels. It provides <30um spatial resolution in both x and y directions, and <25 ps timing resolution per hit. The ETTL has an inner radius of 0.12m and outer radius of 0.64 m and locates right after mRICH at z ~ -1.610 to -1.710 m.

Forward Timing and Tracking Layer (FTTL)
The FTTL consists of one AC-LGAD layer, with 500um x 500 um pixels. It provides <30um spatial resolution in both x and y directions, and <25 ps timing resolution per hit. The FTTL has an inner radius of 0.12m and outer radius of 0.85 m and locates right in front of dRICH at z ~ +1.555 to +1.705 m.

Describe the solution you'd like
This was temporary and needs to be updated

• moreover, I had to add reflectivity properties to the dRICH sensor surface from the ATHENA master branch, in order to see hits; not sure if this is correct (since we did not have this in the ATHENA proposal production)

Tasks:

• check the photon count, to make sure we get the expected numbers
• check the sensor geometry and segmentation
• surface paramaterization (esp. reflectivity)
• sensor bases (how to model the back of the sensors?) - make sure there is enough space for this

Additional context

Notes from ATHENA studies (from discussions with Alexander, Chandra, and Chris):

• sensor used for proposal: https://www.hamamatsu.com/us/en/product/optical-sensors/mppc/mppc_mppc-array/S13361-3050AE-08.html

• the non-negligible gas-to-resin (numerically
  <5%) and resin-to-silicon (~15%) reflections must be effectively
  accounted in the quoted PDEs. At all incident angles of interest
  for us (up to 30 degree or so) there numbers get shared between
  the polarization states, but average stays more or less the same.

• However in our case (g4dRIChOptics.hh) the sensor surface was defined as dielectric_metal with
  some bogus imaginary refractive index parameters. This killed the
  photons, and also I verified that the MC .root files contain only
  the ones which were detected somewhere. So I temporarily changed the material to AirOptical, and added
  a benign surface to the optical_metarials.xml database, and we are
  back at ~10 npe. And "Chandra's number" for 350..650nm integral is
  now ~72 or so.

• I think the correct way to account Cherenkov photon polarization is
  to create a resin volume, and perhaps even a silicon volume inside it,
  but renormalize the PDE, accounting for the normal incident losses.

• look into G4SiPM package

OT but for PRs from forks we could direct users to eicweb where they should submit a pipeline by hand (cumbersome) or write a bot to submit the pipeline when instructed by a comment from a member of epic-devs or epic-admins.

Originally posted by @wdconinc in #39 (comment)

Is your feature request related to a problem? Please describe.
The mpgd layers currently do not have support/service material implemented.

Describe the solution you'd like
Proper materials used for support and services should be implemented. These features include detector supports for the MPGD modules and approximate material for ASICs and cabling.

Describe alternatives you've considered
Some of this material has been incorporated by implementing an additional fudge factor layer whose thickness approximates some of the support/service average material.

These layers for the inner and outer barrel in mpgd_barrel.xml are shown below:

Is your feature request related to a problem? Please describe.
The definition of Polystyrene in compact/materials.xml differs from G4_POLYSTYRENE in Geant4.

Describe the solution you'd like
Change the definition of Polystyrene to replicate G4_POLYSTYRENE -- density 1.06 g/cm^3, 50% C 50% H (Wikipedia says multiples of C8H8), mass fractions of 92.26% and 7.74% for C and H, respectively.
Describe alternatives you've considered
Maintain the current definition of Polystyrene.

Additional context
G4_POLYSTYRENE information from Geant4's material table:

Environment: (where does this bug occur, have you tried other environments)

• main
• HEAD

Steps to reproduce: (give a step by step account of how to trigger the bug)

Look at dump-constants job artifact (constants.toml), or
npdet_info dump ${DETECTOR_PATH}/ecce.xml should work

Expected Result: (what do you expect when you execute the steps above)

According to https://eic.jlab.org/Geometry/Detector/Detector-20220624102507.html:

DIRC_rmax                      =       74.500
DIRC_rmin                      =       71.500 
OuterMPGDBarrel_rmin           =       75.500 (?)
OuterMPGDBarrelLayer_rmax      =       74.500 
OuterMPGDBarrelLayer_rmin      =       76.500 
EcalBarrel_rmin                =       79.500 

Actual Result: (what do you get when you execute the steps above)

DIRC_rmax                      =       78.500 
DIRC_rmin                      =       71.500 
OuterMPGDBarrel_rmin           =       81.000
OuterMPGDBarrelLayer_rmax      =       82.000 
OuterMPGDBarrelLayer_rmin      =       80.000 
EcalBarrel_rmin                =       83.500 

Is your feature request related to a problem? Please describe.
Material property tables for the dRICH are generally obtained from outside this repository in g4dRIChOptics.hh, and copy-pasted into the XML files here in epic. It would be nice to have the parameterizations in g4dRIChOptics.hh stored here in epic somehow.

Describe the solution you'd like
One idea is writing a script that (1) contains or is able to call these parameterizations, (2) generate the tables, and (3) can automatically update the XML files. Step (3) may not be the most useful, if we don't update this table often.

Describe alternatives you've considered
If anything, we should preserve where these numbers came from. More detailed comments in the XML file would be a good start.

Additional context
This header file can dump the tables; if I recall correctly one just needs to run a simulation in Fun4all following https://github.com/cisbani/dRICh documentation

Is your feature request related to a problem? Please describe.
The sensor should be made more realistic.

Describe the solution you'd like

• Add resin base to dRICH sensors, following the S13361-3050NE-08 datasheet
• Use the correct sensor thickness

Environment: (where does this bug occur, have you tried other environments)

• Which branch (main for latest released): main
• Which revision (HEAD for the most recent): HEAD
• Any specific OS or system where the issue occurs? All
• Any special versions of ROOT or Geant4? All

Steps to reproduce: (give a step by step account of how to trigger the bug)

1. cmake and make install

Expected Result: (what do you expect when you execute the steps above)

The epic.xml was thought to be the default configuration, which you get when running the jinja2 template without a specific configuration yaml file. The logic in the template is intended to allow this, but that's not always easy. In several places we have to add things like if features is not defined and stuff to allow running without features. That's quickly turning into a cumbersome approach, and it is not clear to those editing the template that that is what's going on.

Actual Result: (what do you get when you execute the steps above)

We would like to think think that the DIRC is in the default, but it isn't included in epic.xml because the logic is inverted there.

Probable Solution:

We should probably just move away from a default epic.xml, or at least require an explicit configuration for it. This can probably most naturally be achieved by picking a default config argument in bin/make_detector_configuration. We may end up with epic_default.xml and epic.xml being the same, but that's likely clear enough.

For studies related to potential MPGD tracker behind dRICH, the MPGD geometry needs to be implemented. This detector will also need to be segmented in a way the maintains the raw material constraints of the MPGD foils and support materials added.

Is your feature request related to a problem? Please describe.
There have long been some ergnomics issues with Acts integration in DD4hep geometries. These are now addressed in Acts (tentatively targeted for v20.0.0) and build on some recent changes in DD4hep (as of v1.21). For details see acts-project/acts#1257. As is common for Acts, this is backwards incompatible so we're essentially stuck on Acts v19.6.0 until we make the required changes.

Describe the solution you'd like
Instead of segments like (TrapEndcapTracker_geo.cpp)

  // ACTS extension
  {
    Acts::ActsExtension* detWorldExt = new Acts::ActsExtension();
    detWorldExt->addType("endcap", "detector");
    // Add the volume boundary material if configured
    for (xml_coll_t bmat(x_det, _Unicode(boundary_material)); bmat; ++bmat) {
      xml_comp_t x_boundary_material = bmat;
      Acts::xmlToProtoSurfaceMaterial(x_boundary_material, *detWorldExt, "boundary_material");
    }
    sdet.addExtension<Acts::ActsExtension>(detWorldExt);
  }

  // snip

    Acts::ActsExtension* layerExtension = new Acts::ActsExtension();
    layerExtension->addType("sensitive disk", "layer");
    for (xml_coll_t lmat(x_layer, _Unicode(layer_material)); lmat; ++lmat) {
      xml_comp_t x_layer_material = lmat;
      xmlToProtoSurfaceMaterial(x_layer_material, *layerExtension, "layer_material");
    }
    layer_element.addExtension<Acts::ActsExtension>(layerExtension);

we would replace this with segments in xml, e.g.

    <detector
      id="TrackerEndcapP_0_ID"
      name="TrackerEndcapP_Inner"
      type="epic_TrapEndcapTracker"
      readout="TrackerEndcapHits"
      vis="TrackerVis"
      reflect="false">
      <type_flags type="DetType_TRACKER + DetType_ENDCAP" />
      <boundary_material surface="inner" binning="binPhi,binZ" bins0="mat_sol_bPhi" bins1="mat_sol_bZ"/>

      <!-- snip -->

    </detector>

and

  <plugins>
    <plugin name="DD4hep_ParametersPlugin">
      <argument value="TrackerEndcapP_Inner"/>
      <argument value="passive_layer: bool = false"/>
    </plugin>	
  </plugins>

Further details in the new dd4hep plugin documentation page.

Describe alternatives you've considered
For the time being we can remain pinned on Acts v19.6.0 without too many issues, but based on past experience we need a plan in place for transition since incompatible changes pile up quickly.

Is your feature request related to a problem? Please describe.
Currently we have 2 parameterizations available, both using acrylic-substrate mirrors:

1. flat 90% reflectivity (implemented here in epic)
2. data points from measurements of AlMgF2 coated on thermally shaped acrylic sheets, from AJRP, 10/01/2012; this is from Evaristo's common optics class; Silvia mentions that this performance is not nearly as good as we will be able to achieve, and will send us a paper with a better mirror

Describe the solution you'd like
Implement the parameterization from Silvia

Is your feature request related to a problem? Please describe.
Right now we only submit benchmark chains for epic.xml, not for epic_sciglass, epic_imaging, and others such as epic_pfrich. We should submit these different configurations by default and report their outcome as status checks.

Describe the solution you'd like
Matrix benchmark submission on trigger-detector-benchmarks.

Describe alternatives you've considered
Stick to just a single epic.xml submission, which would not allow us to keep track of performance changes in subsystems that are not included in the default.

Additional context
In the past, when geometry was all still on eicweb, we submitted these multiple chains automatically. This was not ported over to github right away (priorities....).

Is your feature request related to a problem? Please describe.
Pull requests from a fork do not have access to the secrets necessary to trigger eicweb benchmarks in the trigger-detector-benchmarks job, and consequently that job fails, e.g. #61. There is no way to 'unfail' this job without going through the API.

Describe the solution you'd like
We should succeed the trigger-detector-benchmarks job, but use it to set the eicweb/detector-benchmarks status to pending. This way, the status will remain pending until someone starts the pipeline by hand, at which point the pipeline will succeed and no remaining failed jobs remain

Describe alternatives you've considered
No other alternatives considered...

Is your feature request related to a problem? Please describe.
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

Describe the solution you'd like
A clear and concise description of what you want to happen.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

Is your feature request related to a problem? Please describe.
There are currently two sets of material definitions, which will lead to overlapping entries (e.g. PbWO4 is moving in that direction with multiple definitions shared between optical and non-optical).

• materials.xml is a definition file for materials using the GDML schema.
• optical_materials.xml uses the lccdd schema.

There is no fundamental reasons to have two files, other than (possibly) readability due to optical property matrices (a possible split which does not lead to duplication could therefore be to keep those optical properties in a separate file).

Furthermore, optical_materials.xml contains more than materials, i.e. it contains optical surfaces.

Describe the solution you'd like

• A single material_properties.xml file with tables of materials.
• A single materials.xml file with all material definitions, which includes elements.xml and material_properties.xml.

Optical surface properties may need to move to dedicated subsystem files where they can be maintained, or need to be made generic. E.g. MirrorSurface_DRICH could be renamed Anomet_Miro_Silver or whatever will be used.

Describe alternatives you've considered
Maintain the status quo...

Additional context
This could be combined with #47.

Is your feature request related to a problem? Please describe.
The ecce compact file does not rely on BEAMLINE_PATH set in
https://github.com/eic/ip6/blob/5fe0178ea0fc6a490c776e02ff750693a60f576f/templates/setup.sh.in#L4
instead, some ad-hoc symlink to ip6 is used.

Describe the solution you'd like

diff --git a/templates/epic.xml.jinja2 b/templates/epic.xml.jinja2
index 5e30dbe..0e08a0c 100644
--- a/templates/epic.xml.jinja2
+++ b/templates/epic.xml.jinja2
@@ -44,8 +44,8 @@
       The ip6 (or other ip) defines should be included first.
       These files have only a define tags.
   </documentation>
-    <include ref="ip6/definitions.xml" />
-    <include ref="ip6/far_forward/fields_{{ pbeam | default("275", true) }}.xml" />
+    <include ref="${BEAMLINE_PATH}/ip6/definitions.xml" />
+    <include ref="${BEAMLINE_PATH}/ip6/far_forward/fields_{{ pbeam | default("275", true) }}.xml" />
     <include ref="compact/definitions.xml" />
   </define>
 

If the variable is not set, DD4hep will fail:

RunPlugin        ERROR ++ Exception while executing plugin DD4hep_CompactLoader:
                dd4hep: Severe error during environment lookup of ${BEAMLINE_PATH}/ip6/definitions.xml Evaluator::Object : unknown variable : ${BEAMLINE_PATH}

There doesn't seem to be a way to provide a default value for ${BEAMLINE_PATH} in DD4hep evaluator, despite some similar feature already working:
https://github.com/AIDASoft/DD4hep/blob/02c6d5ac04c187f37631f9f15ca6c1c08452d992/examples/LHeD/compact/compact_Lhe_dip_sol_ell.xml#L34-L39

Describe alternatives you've considered

• Merge ip6 into this repository

Additional context
Some upstream discussion in AIDASoft/DD4hep#934

Is your feature request related to a problem? Please describe.
A push to a branch with an active pull request triggers two events that can be benchmarked:

• push commit: this is a new event on the branch itself, and it is accessible outside the repository with a regular sha,
• test merge commit: this is a github internal test merge commit into the target branch.

We do a fairly standard thing for pipelines with

on:
  push:
    branches:
      - main
    tags:
      - '*'
  pull_request:

which triggers on the push commit in main and for tags only, and triggers on the test merge commit for pull_requests. The GitHub Actions pipelines are therefore run on the test merge commit.

However, the GitLab pipelines that we spin off on eicweb are effectively run on the push commit since we pass (at best) the branch name or push commit sha to eicweb. Technically we should be passing the test merge commit, which we should get with the following:

git fetch origin +refs/pull/${{ github.pull_request.event.number }}/merge
git checkout FETCH_HEAD

Describe the solution you'd like
This will requires some work on both GitHub and GitLab ends (which we are not doing right now):

• GitHub needs to pass a properly formed ref_name as string
• GitLab needs to clone, fetch the ref, and checkout FETCH_HEAD instead of a simple checkout of sha or branch name

Describe alternatives you've considered
Or we could just be happy with running benchmarks on the branch and merging without testing the test merge commit. Together with requiring that pull requests are always up to date with their target branch, this should ensure that the push commit in the branch is the same as the test merge commit. This approach slows down the pace of development since it serializes all benchmarks. We can at most merge at the rate determined by the slowest required benchmark.

Is your feature request related to a problem? Please describe.
BeAGLE by default cannot be run in dd4hep

Describe the solution you'd like
As long as it runs

Describe alternatives you've considered
No idea.

Additional context

Is your feature request related to a problem? Please describe.
The length of the imaging barrel in the negative z direction should be 257 cm and 177.5 cm in positive direction (from IP) + electronics like in https://eic.jlab.org/Geometry/Detector/Detector-20220929172703.html

Describe the solution you'd like
Currently the calorimeter length is set to:
EcalBarrel_length = 525.000 = EcalBarrelForward_zmax + EcalBarrelBackward_zmax

So either the envelope would need to be changed or the length z and offset in the detector description https://github.com/eic/epic/blob/main/compact/ecal_barrel_interlayers.xml (currently assigned to EcalBarrel_length and EcalBarrel_offset respectively https://github.com/eic/epic/blob/main/compact/ecal_barrel_interlayers.xml#L99).

Is your feature request related to a problem? Please describe.
The current setup of homogeneous detector and the structure of module are too ideal.

Describe the solution you'd like
Add the supporting structure outside(12 side ring) and inside(round ring) of the homogeneous calorimeter detector and place the modules as compacted as possible.

Describe alternatives you've considered

Additional context

The mechanical drawing of EEMC

Environment: (where does this bug occur, have you tried other environments)

• Which branch (main for latest released): main
• Which revision (HEAD for the most recent): HEAD
• Any specific OS or system where the issue occurs?
• Any special versions of ROOT or Geant4?

Steps to reproduce:

The documentation in epic readme is not clear on setting up custom epic/ip6 geometries. If all commands are executed in sequence, after executing the final step source install/setup.sh , you will see the warning that ip6 is not loaded. This implies that local ip6 has not been picked up.

Desired behavior:

The warning should not show and you should be able to set custom epic or ip6 geometry using the setup scripts following the read me. The following setups seem to be allowed after cloning epic and ip6 under the same parent directory and could be added in documentation to improve clarity.

• Default epic+local ip6

source /opt/detector/setup.sh
git clone https://github.com/eic/ip6
cd ip6
cmake -B build -S . -DCMAKE_INSTALL_PREFIX=install
cmake --build build
cmake --install build
source install/setup.sh

• local epic+default ip6

source /opt/detector/setup.sh
git clone https://github.com/eic/epic
cd epic
cmake -B build -S . -DCMAKE_INSTALL_PREFIX=install
cmake --build build
cmake --install build
source install/setup.sh

• local epic+local ip6

git clone https://github.com/eic/ip6
cd ip6
cmake -B build -S . -DCMAKE_INSTALL_PREFIX=install
cmake --build build
cmake --install build
cd ..
git clone https://github.com/eic/epic
cd epic
cmake -B build -S . -DCMAKE_INSTALL_PREFIX=install
cmake --build build
cmake --install build
cd install/share/epic
ln -s ../../../../ip6/install ip6
cd ../../..
source install/setup.sh

N.B.: Maintain consistency between setup script in the container and setup scripts in local repos

Example : The setup script under /opt/detector uses $JUGGLER_BEAMLINE_CONFIG_VERSION whereas the setup script in ip6 local repo install uses $JUGGLER_BEAMLINE_VERSION. Need to make sure that the variable names are consistent.

This detector component measures charge, momentum and spin of all particles.

Is your feature request related to a problem? Please describe.
ECCE is a hold-over from the DPAP days. We should converge around the new name.

Describe the solution you'd like
Rename the repository, rename all occurrences of ECCE in the repository.

Describe alternatives you've considered
We could also fork github.com/eic/ecce into a new name.

Is your feature request related to a problem? Please describe.
The small gap between the aerogel and filter should be made of air; currently it is C2F6 because there is no defined volume between the aerogel and filter, other than the mother gas volume.

Describe the solution you'd like
This can be done by adding a Cone of air, similar to the aerogel and filter Cones (e.g., aerogelSolid). With this being the 3rd such Cone, the repeated code can likely be generalized.

Describe alternatives you've considered
Leave as is; probably won't change anything significantly.

Environment: (where does this bug occur, have you tried other environments)

• Which branch - main

Steps to reproduce: (give a step by step account of how to trigger the bug)

1. make install

Expected Result: (what do you expect when you execute the steps above)

/home/romanov/eic/soft/epic/share/epic/*xml

Actual Result: (what do you get when you execute the steps above)

But I have

/home/romanov/eic/soft/epic/share/ecce