ECal endcap turbine version 3

[varnes]

BEGINRELEASENOTES

This PR is to support v3 of the ECal endcap turbine geometry. In this version there is flexibility to set parameters separately for the three wheels, to calibrate the response in both dimensions along the surface of a blade, and the segment the readout in both directions as well. The readout unit cells are allowed to be smaller than the calibration cells.

(Note that the above changes were implemented in two steps, with the flexibility to set parameters separately introduced in "v2" of the geometry, and the remaining changes in v3. v2 was never included in a pull request.)

ENDRELEASENOTES

[varnes]

Hi @BrieucF ,

Thanks for the suggestions. I'm not quite sure what changes you want for FCCee/ALLEGRO/compact/README.md . There is already a mention of the endcap turbine concept in the notes for v03. What should be changed/added?

On the point about adopting the oX_vY convention for naming the xml: is the suggestion to do that in this PR for all of the xmls, or to just do the endcap turbine ones?

Thanks,
Erich

[varnes]

I've now implemented all of the suggestions from @BrieucF , @giovannimarchiori , and @atolosadelgado (I think). Is there anything else needed from me so that this PR can proceed?

Best regards,
Erich

[giovannimarchiori]

Hi @varnes
it seems the branch is out-of-date, could you just sync it with master please?
Giovanni

[giovannimarchiori]

Hi @varnes
what the heck went wrong with your rebasing in this commit 9587c0a
?
I see old versions of many files appearing now in the MR.... :/

[varnes]

Hi @giovannimarchiori ,

Ugh. I thought that since there probably weren't that many changes since I last rebased, I could get away with just checking out the changed files from main without doing a full rebase (and spot-checking a few of them, I thought it was working). But apparently not... at this point is it best to abandon this MR and try again with a new, rebased, branch?

Best regards,
Erich

[giovannimarchiori]

Hi @varnes
you can try to revert those commits on the remote branch and then once you're back to where you were before we can have a look together.
Your branch was only one commit behind and I expected it would have been quite straightforward to sync it with main just by going to the GitHub web page of your fork, select the proper branch, and click on Sync fork

[varnes]

Hi @giovannimarchiori ,

Indeed life would have been much simpler if I'd been aware of the sync button. I think the PR is in better shape now, as I've followed your advice about reverting the bad commits. Now my branch reports that it is 77 commits ahead of the main branch, and no commits behind. Also the "Files changed" tab on the PR only lists files that I meant to modify.

Please have another look, and apologies for the noise.

Best regards,
Erich

[giovannimarchiori]

Hi @varnes ,

great! I think we're almost there. Could you please update FCCee/ALLEGRO/compact/README.md to mention that in ALLEGRO v03 we updated the ecal endcap turbine to v03 to allow for ... (more details in detector/calorimeter/README.md)?

I would then ask @atolosadelgado to have a final look and approve/merge if it's OK
We'll then have to review https://github.com/varnes/k4RecCalorimeter/tree/ECalEndcap_turbine_v3_topo_try2
Note that also there your branch is behind main so please keep it in sync (same trick as for k4geo) and then we have a look.

Thanks, cheers,
Giovanni

[giovannimarchiori]

@varnes - your branch is out-of-date after a recent merge, please update
@atolosadelgado - are there any changes that you requested that still need to be implemented?
thanks a lot!
Giovanni

[atolosadelgado]

@giovannimarchiori @varnes

(edited, sorry for not checking well before...)

all good from my side. The CI seems to be failing because ALLEGRO test takes too long... can you extend the timeout and/or reduce the number of events?

The CI test also spotted the following:

1039-2025-02-20T00:06:25.1728675Z Compact          INFO  ++ Converted subdetector:EMEC_turbine of type ECalEndcap_Turbine_o1_v03 [SimpleCalorimeterSD]
1040:2025-02-20T00:06:25.1729444Z Evaluator        WARN  +++ Overwriting variable: name=layer_size1 value=100*mm  Evaluator::Object : existing variable [setVariable error]
1041-2025-02-20T00:06:25.1730343Z Detector         WARN  +++ Object 'layer_size1' is already defined. New value will be ignored
1042:2025-02-20T00:06:25.1730981Z Evaluator        WARN  +++ Overwriting variable: name=layer_size2 value=150*mm  Evaluator::Object : existing variable [setVariable error]
1043-2025-02-20T00:06:25.1731586Z Detector         WARN  +++ Object 'layer_size2' is already defined. New value will be ignored
1044:2025-02-20T00:06:25.1732210Z Evaluator        WARN  +++ Overwriting variable: name=layer_size3 value=250*mm  Evaluator::Object : existing variable [setVariable error]
1045-2025-02-20T00:06:25.1732958Z Detector         WARN  +++ Object 'layer_size3' is already defined. New value will be ignored

[giovannimarchiori]

Thanks @atolosadelgado for the feedback - @varnes please have a look at the remaining issues

[varnes]

Hi @atolosadelgado and @giovannimarchiori ,

I've re-synched by branch so hopefully it's up-to-date now. It seems that the issues you found in the CI with the layer_size variables are related to the HCal(?). At least that's the only xml where I see those variables. For the other issue (ALLEGRO test timeout) I've reduced the number of events from 100k to 50k and it seems to be passing now.

[giovannimarchiori]

Hi @atolosadelgado
this PR #437 fixes the problem with the layer_size variables being redefined
It's a simple one where I only fixed the two xml files that were both defining the layer_size constants (for different sub detectors)