Adding FCC-SEED as option to all detectors and refactor IDEA/ALLEGRO vertex detector

[armin-ilg]

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

• Adding FCC-SEED vertex detector layout as option to ILD (see Adding truth info for FCC-ee ILD model, add vertex resolutions as inp… iLCSoft/ILDConfig#188), ALLEGRO, IDEA, and CLD (option CLD_o5_v01)
• Fixing CLD disk material budget from CLD_o2_v09 and CLD_o5_v01 on (Half the material budget in CLD vertex disks #613)
• Updating ALLEGRO vertex detector and silicon wrapper properties: 20x20 µm^2 pixels everywhere in vertex, 50x50 µm^2 pitch pixels in SiWr, only one layer of SiWr (see [ALLEGRO] - Silicon wrapper and vertex assumptions in FCC-config and k4geo #617)
• Refactoring IDEA/ALLEGRO vertex detector. Inner vertex, outer vertex, and disks are now in separate xml files and can be flexibly combined (see VertexComplete_o1_v04.xml). Ultra-light inner vertex is in a standalone xml file (VertexInnerBarrelUltraLight_o1_v04.xml)

ENDRELEASENOTES

[Zehvogel]

For CLD I think it would fit our versioning scheme better if you add the FCC-SEED as a new option, i.e. CLD_o5_v01 :)

[armin-ilg]

For CLD I think it would fit our versioning scheme better if you add the FCC-SEED as a new option, i.e. CLD_o5_v01 :)

The FCC-SEED option is now added as CLD_o5_v01, and the CLD disk material budget fix is CLD_o1_v09 (see #613)

[armin-ilg]

Hi. The PR is now ready for review.

@Victor-Schwan @tmadlener : Do you want FCC-SEED to be the default vertex detector in ILD?

@BrieucF @giovannimarchiori : This PR includes the changes for ALLEGRO (pixel pitches and only one layer of Silicon Wrapper). FCC-SEED can be used for the inner vertex if desired.

@lopezzot : For IDEA only the structure of the vertex detector xml file is changed. One can, however, also use FCC-SEED as inner vertex if desired.

@andresailer @Zehvogel : The FCC-SEED option is now added as CLD_o5_v01, and the CLD disk material budget fix is CLD_o1_v09 (see #613).

[Zehvogel]

Hi @armin-ilg thank you for the contribution!

This is a particularly CLD-centric problem, but I think the PR would benefit from a cleanup of the commit history.
In the case of changes to CLD, we would like a structure similar to

1. Copy version X as basis for version Y
2. Add feature Z to version Y
   Otherwise, it is very difficult to perform the review.

Let me know if you have questions on how to do this. :)

[tmadlener]

@Victor-Schwan @tmadlener : Do you want FCC-SEED to be the default vertex detector in ILD?

I think for now we prefer to stay with the current version to not introduce yet another moving part into our efforts of getting a full tracking/reconstruction chain going. We would then switch to this once we have a (technical) baseline in place.

Unrelated to ILD, this will almost certainly break the conversion to an Acts geometry that happens in the k4ActsTracking, so also the blueprint building in https://github.com/key4hep/k4ActsTracking/blob/main/k4ActsTracking/src/components/DD4hepBlueprintConstruction.cpp will have to be adapated accordingly.

[armin-ilg]

@Victor-Schwan @tmadlener : Do you want FCC-SEED to be the default vertex detector in ILD?

I think for now we prefer to stay with the current version to not introduce yet another moving part into our efforts of getting a full tracking/reconstruction chain going. We would then switch to this once we have a (technical) baseline in place.

Unrelated to ILD, this will almost certainly break the conversion to an Acts geometry that happens in the k4ActsTracking, so also the blueprint building in https://github.com/key4hep/k4ActsTracking/blob/main/k4ActsTracking/src/components/DD4hepBlueprintConstruction.cpp will have to be adapated accordingly.

Thanks for the feedback, I changed the default back to the CLD vertex (the one from CLD_o2_v09 with the fixed disk mat. budget).

Indeed k4ActsTracking doesn't work yet with all detectors using VertexBarrel_detailed/VertexEndcap_detailed. This is on my to do list and I would tackle it in a future PR (just need to add the two blueprints as you pointed out).

[armin-ilg]

Hi @armin-ilg thank you for the contribution!

This is a particularly CLD-centric problem, but I think the PR would benefit from a cleanup of the commit history. In the case of changes to CLD, we would like a structure similar to

1. Copy version X as basis for version Y
2. Add feature Z to version Y
   Otherwise, it is very difficult to perform the review.

Let me know if you have questions on how to do this. :)

Hi @Zehvogel

I changed the commits as you described, with one commit copying the files from CLD_o2_v08 and the other one making changes on top for CLD_o5_v01 and CLD_o2_v09 (could certainly be improved further, but it's a bit complicated with so many commits over a long time). Hope this is okay now.

[Zehvogel]

Hi @armin-ilg, I appreciate the attempt, but this still looks like I will not be able to review it in a reasonable time. If you would rebase your changes on top of the current main, instead of this merge commit chaos, I think the history would approach being readable...

I know that it can be a very annoying process, especially with as many changes as this, but otherwise I have to try to perform this restructuring myself (either in my head or manually) to provide a meaningful review. The set of changes is too big to do it in my head, and I don't have the time in the near future to do it manually.

If it is already too complicated for you, who knows what the changes are, to structure them because it is too much at once, then how is someone without this insider knowledge supposed to understand what the changes are?

[armin-ilg]

Hi @armin-ilg, thanks a lot! The changes for ALLEGRO (resolutions, 1 layer in Si-wrapper) are consistent with the requests from the tracker group. I would suggest to document these modifications in https://github.com/key4hep/k4geo/tree/main/FCCee/ALLEGRO/compact/README.md.

It is not clear to me how one should have to change the ALLEGRO compact files to have the FCC-seed vertex detector replace the current one - I wonder if it could be inserted as a commented code in the xml, or if one could add some mention of it in the README.

Done, thank you, this somehow got lost in tidying up the PR. Here's the commit: 0ea9c41

[giovannimarchiori]

looks good to me from ALLEGRO's perspective

[giovannimarchiori]

Ciao @lopezzot - would you have time to review this PR from IDEA's side?

@Zehvogel what about CLD?

[lopezzot]

Ciao @lopezzot - would you have time to review this PR from IDEA's side?

Hi @giovannimarchiori, thank you for the question, but unfortunately I am not directly involved in the IDEA simulation anymore as I changed institute.

[SanghyunKo]

@lopezzot @giovannimarchiori Hello guys, we discussed about this in yesterday's full simulation meeting, and I'll take a look from the IDEA side. Thanks for the message though.

[SanghyunKo]

The refactoring part for IDEA generally looks okay. But my codex found a couple of potential bugs: