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

CLD with ARC #289

Merged
merged 32 commits into from
Nov 22, 2023
Merged

CLD with ARC #289

merged 32 commits into from
Nov 22, 2023

Conversation

atolosadelgado
Copy link
Contributor

@atolosadelgado atolosadelgado commented Sep 6, 2023

BEGINRELEASENOTES

  • CLD_o3_v05 is created. This option evolves from option 2 version 05. The main characteristics with respect the mother version are:
    • ARC subdetectors included between the trackers and the ECal.
    • Trackers are shrunk by 20 cm in radial and Z direction to accommodate the ARC. The space between layers is reduced proportionally, and the angular coverage is preserved.

ENDRELEASENOTES

Hi,

components from CLD (o2, v05) were taken to build the option 3, which contains the ARC detector, between the tracker and the ECAL. Modifications to the size of the outer tracker barrel, outer tracker endcap and inner tracker endcap were detailed in this presentation.

Overlap check with 10k points passed (it took 4h).

I am doing now some efficiency plots, but I think it was a good moment to open the PR.

@atolosadelgado
Copy link
Contributor Author

I just noticed that the volume names, constant names, and the readout name for the ARC do not match the style of the CLD.

I will change it right away, I only need some minutes to do it.

@atolosadelgado
Copy link
Contributor Author

Changes in the naming are done, tests passed

@atolosadelgado
Copy link
Contributor Author

Should some release note comments be filled in the OP?

@andresailer andresailer self-requested a review October 3, 2023 13:46
alvarotd and others added 22 commits October 3, 2023 17:08
Now total length over Z is from z=-2m to z=2m
Support of tracker also reduced. Now inner tracker extends up to 2m in Z
and Z-thickness of the gas is now ENDCAP thickness - wall thickness
Fixed bug: sensor of reflected cells are z-centered OK
so there is no overlap with ARC
Overlap check with 10k points passed OK
@atolosadelgado atolosadelgado mentioned this pull request Nov 13, 2023
@jmcarcell
Copy link
Contributor

Can you fix the conflict @atolosadelgado? It's a very simple one line in the README

@atolosadelgado
Copy link
Contributor Author

I still do not understand why I get different simulation output if I use LCG, Key4hep or my local DD4hep, but since it is surely unrelated to this repository, please feel free to merge this PR at your earlier convenience :)

@atolosadelgado
Copy link
Contributor Author

Please, do not merge, I have just found overlap of sensor in cell 11 of the barrel... sorry for the noise

@jmcarcell
Copy link
Contributor

@atolosadelgado if you have Alma 9 the latest nightlies have Geant4 without Vecgeom. CentOS 7 will have to wait a bit and all the workflows here are CentOS 7...

@atolosadelgado
Copy link
Contributor Author

atolosadelgado commented Nov 21, 2023

thank you very much! I just tested the overlaps in lxplus9 and now the test passes :):)

This is the code to reproduce the test,

source /cvmfs/sw-nightlies.hsf.org/key4hep/setup.sh
cd /tmp/
git clone -b CLD_with_ARC [email protected]:atolosadelgado/k4geo.git
cd k4geo/
cmake -B build -S . -D CMAKE_INSTALL_PREFIX=install
cmake --build build -j 4 -- install
export LD_LIBRARY_PATH=$PWD/install/lib:$LD_LIBRARY_PATH
cd build/
ctest -V -R t_test_ARC_o1_v01_overlap

At the beginning I though the problem was the unit system and I added a dummy offset in sensor position to try to fix the overlap problem. It turns out that the problem is VecGeom and that offset is not needed. I have removed that ad-hoc offset in the most recent commit. I believe this PR can be merged now.

@andresailer
Copy link
Contributor

Now there is a test failure with the overlap check

@jmcarcell
Copy link
Contributor

jmcarcell commented Nov 21, 2023

LCG_100 has Geant4 with Vecgeom enabled, which could explain the issues. The CI test with the nightlies just passed (CentOS 7 - Geant4 without Vecgeom since a while ago).

@atolosadelgado
Copy link
Contributor Author

Thanks Juan for the clarification. I have run the overlap test in centos 7 (LCG latest view dev4) and alma9 (LCG and key4hep) and the test passes.
LCG 103 and before use Geant4 with VecGeom, so the overlap check will not pass.

@andresailer
Copy link
Contributor

Ok, we should update the stacks used for the tests then.

@jmcarcell
Copy link
Contributor

Here or can we merge this PR @andresailer ? k4geo is one of the few missing tags

@andresailer
Copy link
Contributor

Yes, let's merge it and update the CI bases after

@jmcarcell jmcarcell merged commit 85082ec into key4hep:master Nov 22, 2023
2 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