Skip to content

Don't have a default for the geo tool.#223

Merged
giovannimarchiori merged 7 commits intoHEP-FCC:mainfrom
scott-snyder:nodefgeotool-20260323
Apr 9, 2026
Merged

Don't have a default for the geo tool.#223
giovannimarchiori merged 7 commits intoHEP-FCC:mainfrom
scott-snyder:nodefgeotool-20260323

Conversation

@scott-snyder
Copy link
Copy Markdown
Collaborator

Remove the default value for the geometry tool from Create[Positioned]CaloCells. If a value is given, Gaudi will try to create the tool, even if it is never retrieved. But in that case, it probably hasn't been configured properly, which can result in problems later.

BEGINRELEASENOTES

  • Remove default value for the geometry tool from CreateCaloCells/CreatePositionedCaloCells.
    ENDRELEASENOTES

scott-snyder and others added 2 commits March 23, 2026 19:00
Remove the default value for the geometry tool from Create[Positioned]CaloCells.
If a value is given, Gaudi will try to create the tool, even if it is
never retrieved.  But in that case, it probably hasn't been configured
properly, which can result in problems later.
@giovannimarchiori
Copy link
Copy Markdown
Contributor

@scott-snyder when we worked on code like this in the past 2-3 years we tried, as suggested by @BrieucF , not to break legacy digi/reco scripts used for FCC-hh studies, that's why I guess we have the PhiEta tube layer tool by default here.
I am fine with removing that, but then maybe we should make sure that then the tool is configured properly in the scripts used for FCC-hh studies => Brieuc, any guidance here?

@scott-snyder
Copy link
Copy Markdown
Collaborator Author

hi -

The only case that this would potentially break is if one is creating cells
with noise/crosstalk enabled but did not specify the geometry tool.
And i'm not sure that that would have worked anyway, since the geometry
tool would not have the readout configured.

That in fact relates to the motivation for this change. A follow on change
deduplicates the lists of cells held by the calorimeter tool, but the
implementation of that doesn't work if there's no readout name, since that's
used as a key to identify the segmentation. So creating instances of the
tool without the readout configured was causing problems --- but the tool
is anyway only used in the case of noise/crosstalk.

I'm sympathetic to keeping fcchh code working, but if it hasn't been
regularly tested, then it's likely already broken. Is there an example
somewhere that's anywhere close to working? So far i haven't found one.

There are a couple fcchh tests in FCCSW, but those seem to be simulation only,
not reconstruction. There are some tests in k4RecCalorimeter under
RecFCChhCalorimeter, but those were disabled five years ago; a quick
test doesn't get very far at all. And the reco example anyway doesn't
use noise/crosstalk so this change shouldn't affect it anyway.

It might be a good idea to revive some of these fcchh tests and add
them to CI, but that would be a separate task.

@BrieucF
Copy link
Copy Markdown
Contributor

BrieucF commented Mar 27, 2026

Hi!

Here is an example of FCC-hh steering file that we indeed tried to keep alive even if not much used recently. The tests got disable, I just re-launched them. Last time they ran (Sept 2025), the Alma9 ones succeeded.

@scott-snyder
Copy link
Copy Markdown
Collaborator Author

hi Brieuc -

The file you pointed to appears to be FCCee/IDEA.
I assume you meant maybe geant_fullsim_fcchh_main.py?
I had seen that, but that appears to be only the simulation step,
not reconstruction as is discussed here.
(It also uses the k4run simulation setup, not ddsim.)

I made a go at running simulation using that script and then trying to
run reconstruction using RecFCChhCalorimeter/tests/options/recoPositions_fullCaloSystem.py,
which does at least the cell reconstruction.

I immediately ran into the issue that the k4RecCalorimeter code now looks
for the k4geo versions of the segmentation classes, but the FCChh description
in FCCDetectors is written using the pre-k4geo versions of the segmentations.
If i patch up the xml files, i run into the two additional issues that
the reco script is trying to process the hcal extended barrel/endcap, but
those are not made by the simulation script, and also the container names
don't match (simulation writes containers ending in Hits, while reco looks
for containers ending in Cells). If i fix these issues up as well, the
reconstruction works (or at least doesn't crash, since i haven't actually
checked the output).

In any case, if this is something we actually want to keep alive, then
the FCChh configurations probably need to be added to k4geo.
Is that something i should try to do?

@BrieucF
Copy link
Copy Markdown
Contributor

BrieucF commented Mar 30, 2026

Hi Scott, thanks for looking into this! Yes, it would be great to revive the FCC-hh tests and to migrate its components to match the new code organization (e.g. the geometry still lives in FCCDetectors). But it can indeed be done as a separated task. One difficulty I anticipate is that we can't easily migrate to ddsim for FCC-hh workflows because of RedoSegmentation: it has to be adapted to run on CaloHitContribution (the k4run simulation was storing SimCalorimeterHits with infinite granularity), which is also something interesting to do.

@scott-snyder
Copy link
Copy Markdown
Collaborator Author

ping

@scott-snyder
Copy link
Copy Markdown
Collaborator Author

I haven't found any tests that this breaks, and this has a conflict with the migration of the last header from k4FWCore.
Be great to get this merged.

@giovannimarchiori giovannimarchiori enabled auto-merge (squash) April 9, 2026 08:04
@giovannimarchiori giovannimarchiori merged commit 0a7b9a9 into HEP-FCC:main Apr 9, 2026
2 of 3 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.

3 participants