-
Notifications
You must be signed in to change notification settings - Fork 34
Flag clusters as EMCal or HCal #2078
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
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/17840742667. Please merge this PR into the branch `for-calo-type-flag` to resolve failures in PR #2078. Auto-generated by [create-pull-request][1] [1]: https://github.com/peter-evans/create-pull-request Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clang-Tidy found issue(s) with the introduced code (1/1)
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
No Clang-Tidy warnings found so I assume my comments were addressed
| enum ClusterType : int32_t { | ||
| kCluster2D = 0, | ||
| kCluster3D = 1, | ||
| kClusterSlice = 2, | ||
| kClusterEMCal = 3, | ||
| kClusterHCal = 4 | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a mutually exclusive enumeration. What about an Ecal cluster makes it impossible to be a 2D or 3D cluster?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct! Ultimately, I think a longer-term, more flexible approach would be to treat the type as a bit field where we could turn on/off bits for 2D, 3D, EMCal, HCal, etc. I say a few words about this in EDM4eic#122.
My rationale in just extending the enum here rather than proceeding with the bit-field approach is that this is a minimal change that still satisfies what's needed for PFA3 (a generic way to identify EMCal vs. HCal clusters). This would allow for PF development to proceed in parallel with exploring better, long-term solutions.
wdconinc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be encoding Ecal vs Hcal clusters in the data type since it is accessible from the detector type in DD4hep, e.g. https://github.com/AIDASoft/DD4hep/blob/e369490a18a9857d5a61771f340c1791e8f9f7a3/DDCore/include/DD4hep/DetType.h#L53-L54
Ah! I wasn't aware of this But then I'm curious about how I would use this downstream. For example, would I extract these from a cell ID? Or would I look them up based on the system ID? In the former case, I suspect I would need to select a hit from the cluster to identify a cell ID; and in the latter, I expect I would need to provide the system IDs to use as a parameter to the algorithm... |
Yeah, it's only imposed by ACTS for tracking, which is why those have it...
Cell ID would give you system ID as the 8 lsb. Presumably there is nothing in our data model that is so disconnected from hits that you can't get there in a hop or two. If the use is for within EICrecon, then I'd prefer to not just add unspecific bit fields to data model. I don't know what it adds to the end user if they know it's a 2D vs 3D cluster. And within EICrecon at what point is there confusion about this, and does it matter? In any case, having to specify what an algorithm produces in its configuration seems redundant. |
|
Gotcha! Makes sense! The use case I was imagining was primarily in EICrecon (e.g. as a cross-check in a topoclustering algorithm that expects to ingest only 2D clusters, or in algorithms where you need to divvy up energy by calorimeter type), but the EM/Had distinction was one that could be useful for downstream analysis... Either way, if there's an easy way to extract it from DD4hep then we can just go with that! Just to check before opening a PR to add them: the EMCal and HCal DD4hep tags shouldn't impact the actual detector simulation, right? They're just there for description? |
Should not impact simulation indeed. May impact overall event classification. This is used to indicate in an event bit pattern whether the event had any energy deposition in an Ecal or Hcal, but we don't do anything with that. |
Briefly, what does this PR introduce?
This PR introduces two new flags to the
Jug::Reco::ClusterTypeinalgorithms/calorimetry/ClusterTypes,kClusterEMCalandkClusterHCal. These flags are intended to help facilitate generic algorithms downstream which need to distinguish between EM and hadronic energy.These are new types are used to flag only the types of clusters which will be passed on to downstream, holistic algorithms such as the output of
CalorimeterClusterRecoCoG(for all calorimeters other than the BIC) orEnergyPositionClusterMerger(for the BIC).Note: this is intended to be an interim solution, aimed at facilitating development on downstream PF algorithms and maintaining momentum (see the presentation here for more details on the PF development plan). A longer-term, more flexible approach is discussed in EDM4eic#122.
What kind of change does this PR introduce?
Please check if this PR fulfills the following:
Does this PR introduce breaking changes? What changes might users need to make to their code?
No.
Does this PR change default behavior?
No.