Skip to content

Conversation

tonnysoyyo
Copy link

Briefly, what does this PR introduce?

This PR adds a new CalorimeterEoverPCut factory and algorithm to the BEMC reconstruction plugin. It computes an event‑by‑event E/p ratio for each barrel calorimeter cluster (using the highest‑weight MC association for true momentum), applies a JANA‑configurable threshold (ecut) and optional max_layer, and emits an edm4hep::ParticleIDCollection marking clusters that pass the cut as electrons. The factory will be registered in BEMC.cc under the EDM4EIC_VERSION_MAJOR >= 8 guard, mirroring the existing PreML/PostML pattern for EEMC.cc.

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No breaking changes. Existing plugins and factories are unaffected.

Does this PR change default behavior?

No, no existing plugins or factories have been affected

@tonnysoyyo tonnysoyyo linked an issue Jul 9, 2025 that may be closed by this pull request
@github-actions github-actions bot added the topic: calorimetry relates to calorimetry label Jul 9, 2025
@tonnysoyyo tonnysoyyo requested a review from wdconinc July 9, 2025 04:50
@tonnysoyyo tonnysoyyo self-assigned this Jul 9, 2025
@@ -0,0 +1,56 @@
#include <edm4eic/EDM4eicVersion.h>
#if EDM4EIC_VERSION_MAJOR >= 8
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to guard this for older versions. This is mostly done for features that require data model evolution. Here you are using standard functionality only.

Comment on lines 15 to 18
if (!assoc_opt) {
warning("[E/P Cut] no MC associations, skipping");
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes it non-optional, so you should not mark is as std::optional in the header file.

}

// true momentum and E/P
double ptrack = edm4hep::utils::magnitude(bestAssoc.getSim().getMomentum());
Copy link
Contributor

Choose a reason for hiding this comment

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

This is using truth-level information in a non-transparent way. We should use the reconstructed momentum (see coincidentally #1960).

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we can use truth for now, but explicitly ingest the MCParticleCollection as input too.


// true momentum and E/P
double ptrack = edm4hep::utils::magnitude(bestAssoc.getSim().getMomentum());
double ep = (ptrack > 0 ? cluster.getEnergy() / ptrack : 0.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is the layer information used? This is using the full energy of the entire cluster, not the energy deposited up to a certain layer, as intended.

Comment on lines 26 to 33
// pull params from JANA: BEMCEoverPCut:ecut , BEMCEoverPCut:max_layer
double ecut_val = m_algo->getEcut(); // initial default
GetApplication()->GetParameter(GetPrefix() + ":ecut", ecut_val);
m_algo->setEcut(ecut_val);

int maxL = m_algo->getMaxLayer(); // initial default
GetApplication()->GetParameter(GetPrefix() + ":max_layer", maxL);
m_algo->setMaxLayer(maxL);
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to leave this similar to the other factories, i.e. use ParameterRef instead of handling parameter setting by hand.

const auto& [clusters_notnull, track_matches_notnull, hits_notnull] = input;
auto const& clusters = *clusters_notnull;
auto const& track_matches = *track_matches_notnull;
auto const& hits = *hits_notnull;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an unused variable and causes the compilation to fail in the checks.

Copy link
Contributor

@github-actions github-actions bot left a 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)

target_tensor.addToShape(2); // is electron, is hadron
target_tensor.setElementType(7); // 7 - int64
std::vector<edm4eic::Cluster> sel_clusters;
if (!ep_pids) {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-implicit-bool-conversion ⚠️
implicit conversion __tuple_element_t<2UL, tuple<not_null<const ClusterCollection *>, const MCRecoClusterParticleAssociationCollection *, const ParticleIDCollection *>> (aka const edm4hep::ParticleIDCollection *) -> bool

Suggested change
if (!ep_pids) {
if (ep_pids == nullptr) {

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

Todo:

  • move this file to github.com/eic/epic-data
  • include as calibration artifact in github.com/eic/epic

Copy link
Contributor

@github-actions github-actions bot left a 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)

{
// FIXME: use track momentum once matching to tracks becomes available
edm4eic::MCRecoClusterParticleAssociation best_assoc;
for (auto assoc : *cluster_assocs) {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ clang-analyzer-core.NullDereference ⚠️
Dereference of null pointer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

E/p cut for e/pi separation
2 participants