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

Fix segmentation position calculation for Allegro ECal. #423

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

scott-snyder
Copy link
Contributor

Fix the calculation of cell position for
FCCSWGridModuleThetaMerged_k4geo.
Also add a layer() method to that class to extract the layer from a cell ID.

BEGINRELEASENOTES

  • Fix cell position calculation for the Allegro ECal.
    ENDRELEASENOTES

@scott-snyder
Copy link
Contributor Author

Depends on PR#1384 from DD4hep (AIDASoft/DD4hep#1384), and requires a corresponding change in k4RecCalorimeter.

@atolosadelgado
Copy link
Collaborator

Hi @scott-snyder

Thank you for your PR.

I think your approach is correct. However, I was wondering if it makes more sense that this extra information is stored as a data extension of the detector element, to be later retrieved by the segmentation itself and copied internally.

A data extension would be easier to retrieve later during the reconstruction steps. @BrieucF @giovannimarchiori do you have idea about some key information that you would like to attach to the ECAL? I am not sure it is the best reference, but for example this is the data extension for the drift chamber [link]

Nevertheless, I remember what you noticed that the initialization of the internal cache of the segmentation can not happen within the constructor, so it has to be done later. I think that step is unavoidable.

@atolosadelgado
Copy link
Collaborator

Hi,

The detector geometry appears to be accessible through the handle [link].

If you could store the geometry details as a data extension of the detector element, it could provide a more elegant solution. Please let me know if you require further support.

@giovannimarchiori
Copy link
Contributor

Hi @atolosadelgado,

we have layer radial info stored as dd4hep::rec::LayeredCalorimeterData in the C++ builder of the detector

dd4hep::rec::LayeredCalorimeterData::Layer caloLayer;

Is this the "data extension" you are proposing, or is it something different?
Cheers,
Giovanni

@atolosadelgado
Copy link
Collaborator

atolosadelgado commented Jan 23, 2025

Hi @atolosadelgado,

we have layer radial info stored as dd4hep::rec::LayeredCalorimeterData in the C++ builder of the detector

dd4hep::rec::LayeredCalorimeterData::Layer caloLayer;

Is this the "data extension" you are proposing, or is it something different? Cheers, Giovanni

Yes, that is. This subdetector has a data extension of type dd4hep::rec::LayeredCalorimeterData, attached few lines before [link]

My point is that the segmentation seems capable of retrieving the detector element and its data extension. Therefore, I’m wondering if the code in this PR, which creates an internal object of type std::vector<LayerInfo> and performs calculations within the segmentation, is truly necessary. Could the segmentation instead directly utilize the LayeredCalorimeterData provided by the geometry? This way, the code would become simpler and ensure consistency with the geometry.

@scott-snyder
Copy link
Contributor Author

Thanks for the comments and suggestion. I'll take a look and see if that approach is workable.

Fix the calculation of cell position for FCCSWGridModuleThetaMerged_k4geo.
Also add a layer() method to that class to extract the layer from a cell ID.
@scott-snyder
Copy link
Contributor Author

hi -

So here's a stab at storing the derived geometry information as DetElement
extensions rather than in the segmentation class.

The code is a bit nicer this way. The tradeoff is that it is very roughly
a factor of two slower than the previous version, due to the extra lookups.
It also now shorn of any pretence of thread-safety (as the dd4hep
extension mechanism is not thread-safe).

As far as LayeredCalorimeterData, it doesn't have the information we want
to cache (and we don't want to recompute it each time position is called).
And i don't think it should be added to the version in dd4hep.
So we use an additional extension.

An alternative would be to have the extension filled in createECalBarrelInclined
when the geometry is created, rather than lazily in the segmentation.
But the information needed is specific to the segmentation (it depends
for example on the phi ganging), so i don't think that really makes sense.

@atolosadelgado
Copy link
Collaborator

hi -

So here's a stab at storing the derived geometry information as DetElement extensions rather than in the segmentation class.

The code is a bit nicer this way. The tradeoff is that it is very roughly a factor of two slower than the previous version, due to the extra lookups. It also now shorn of any pretence of thread-safety (as the dd4hep extension mechanism is not thread-safe).

As far as LayeredCalorimeterData, it doesn't have the information we want to cache (and we don't want to recompute it each time position is called). And i don't think it should be added to the version in dd4hep. So we use an additional extension.

An alternative would be to have the extension filled in createECalBarrelInclined when the geometry is created, rather than lazily in the segmentation. But the information needed is specific to the segmentation (it depends for example on the phi ganging), so i don't think that really makes sense.

Hi,

You are right that the data extension is expected to be initialized together with the geometry builder. If it can not be used as read-only information by the segmentation, then it is not a good solution, I am sorry I misunderstood what the code was doing.

If you roll back to your original implementation, I can merge it.

Thank you for your time.

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