Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions src/algorithms/calorimetry/SimCalorimeterHitProcessor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -70,17 +70,32 @@ template <> struct hash<std::tuple<edm4hep::MCParticle, uint64_t, int>> {

// unnamed namespace for internal utility
namespace {
// Lookup primary MCParticle @TODO this should be a shared utiliy function in the edm4xxx
// Lookup primary MCParticle
// we stop looking if we find the parent has status 1 but with only 2 daughters
// so we don't merge radiative photons with the primary electron as this prevents us
// from properly linking the clusters back to the event geometry. Note that we also
// enforce for this case that no steps back towards the primary were taken to avoid
// storing the first pair of calorimetric showers that start inside the tracking volume.
// Hence, this algorithm will return:
// - Contribution came from primary: primary
// - Contribution came from immediate daughter of primary and has no childern -> daughter
// - All other cases (i.e. early showers, multi-radiation): primary
//
// @TODO this should be a shared utiliy function in the edm4xxx
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you file this as an issue in edm4eic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do - there are multiple copies of this function across multiple algorithms that we could get rid of.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Been meaning to ask copilot for code duplication... And enable that as a pre-commit or clang-tidy check...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should modify other copies too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure as I don't know how they are used - I could see a case where this function could be somewhat configurable depending on use case - it wouldn't be too hard to add a parameter (could be templated or not) to steer the behavior. And/or have multiple versions with explicit different names that should be called by the user for clarity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also doesn't make sense to have different definitions under the same name.

I would just make the change and @ruse-traveler can confirm about other use cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed! I'd also say to go ahead and make the change! (A separate PR for the copies will be fine, naturally)

I think the use-case here lines up with the intent behind the other cases, and I also think this is a very useful distinction to draw at the truth level. My thinking is that for cases like these, it would be the job of holistic algorithms (like PF) to reconstruct the photon. Thus reconstructed-generated mapping between the 2 cases would be:

  • cluster <--> electron
  • reco particle <--> photon

// libraries
edm4hep::MCParticle lookup_primary(const edm4hep::CaloHitContribution& contrib) {
const auto contributor = contrib.getParticle();

edm4hep::MCParticle primary = contributor;
size_t steps_taken = 0; // The number of steps taken looking for the primary
while (primary.parents_size() > 0) {
if (primary.getGeneratorStatus() != 0) {
auto parent = primary.getParents(0);
if (primary.getGeneratorStatus() != 0 ||
(parent.getGeneratorStatus() != 0 && parent.daughters_size() == 2 && steps_taken == 0)) {
break;
}
primary = primary.getParents(0);
primary = parent;
steps_taken += 1;
}
return primary;
}
Expand Down
Loading