Skip to content

Add track relationship to TaggerTrackerReconstructedParticles - #2841

Open
simonge wants to merge 19 commits into
mainfrom
fix/vertex_traj_protect
Open

Add track relationship to TaggerTrackerReconstructedParticles#2841
simonge wants to merge 19 commits into
mainfrom
fix/vertex_traj_protect

Conversation

@simonge

@simonge simonge commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Briefly, what does this PR introduce? Please link to any relevant presentations or discussions.

Adds the relationship between the ReconstructedParticle and the Track which was used to reconstruct it in the LOWQ2 workflow.
Currently these tracks do not have a trajectory associated with them so downstream algorithms which request the trajectory directly have had a protection added before them.

What is the urgency of this PR?

  • High (please describe reason below)
  • Medium
  • Low

What kind of change does this PR introduce?

Please check if any of the following apply

  • This PR requires changes to geometry (epic PR: __)
  • This PR requires changes to EDM4eic (EDM PR: __)
  • This PR introduces breaking changes. Please describe changes users need to make below.
  • This PR changes default behavior. Please describe changes below.
  • AI was used in preparing this PR. Please describe usage below.

ReconstructedParticle-Track relationship will be populated for Low-Q2 Tagger electrons

Copilot AI lite review requested due to automatic review settings August 7, 2026 14:47
@github-actions github-actions Bot added topic: tracking Relates to tracking reconstruction topic: far-backward Reconstruction related to far backward detectors labels Aug 7, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the LOWQ2 TaggerTracker “Transportation” chain to carry the ReconstructedParticle ↔ Track relationship through the post-ML step, and adds defensive checks in several downstream algorithms to handle tracks that may not have an associated trajectory in this workflow.

Changes:

  • Wire TaggerTrackerLocalTracks into FarDetectorTransportationPostML and attach each output ReconstructedParticle to its corresponding input Track.
  • Update LOWQ2 plugin wiring to pass the additional track collection into TaggerTrackerTransportationPostML.
  • Add Trajectory::isAvailable() guards in multiple tracking/reco algorithms to avoid accessing missing trajectories.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/factories/fardetectors/FarDetectorTransportationPostML_factory.h Adds a Track PodioInput and forwards it into the algorithm.
src/detectors/LOWQ2/LOWQ2.cc Updates the LOWQ2 factory generator wiring to pass TaggerTrackerLocalTracks into PostML.
src/algorithms/fardetectors/FarDetectorTransportationPostML.h Extends algorithm input signature to include a TrackCollection.
src/algorithms/fardetectors/FarDetectorTransportationPostML.cc Populates the output particle’s tracks relation from the input track collection.
src/algorithms/tracking/TracksToParticles.cc Skips tracks lacking trajectories to avoid invalid trajectory access.
src/algorithms/tracking/SecondaryVertexFinder.cc Skips tracks lacking trajectories before iterating track parameters.
src/algorithms/tracking/IterativeVertexFinder.cc Skips tracks lacking trajectories before iterating track parameters.
src/algorithms/reco/Helix.cc Skips unavailable trajectories when building a helix from reconstructed-particle tracks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/algorithms/reco/Helix.cc
Comment thread src/algorithms/fardetectors/FarDetectorTransportationPostML.cc Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 7, 2026 15:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/algorithms/fardetectors/FarDetectorTransportationPostML.cc:115

  • The output particle is created and populated before validating that the corresponding input track exists. If the bounds/availability checks fail, the code will throw after having already created an output object, which can leave partially-populated collections if exceptions are caught upstream and makes debugging harder. Validate tracks and the index before calling out_particles->create().
    particle.setMass(m_mass);
    particle.setPDG(m_cfg.pdg_value);

    if (tracks == nullptr) {
      error("No tracks collection provided; cannot set ReconstructedParticle-Track relation");

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

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)

Comment thread src/algorithms/tracking/TracksToParticles.cc Outdated
Comment thread src/algorithms/tracking/TracksToParticles.cc
Comment thread src/algorithms/tracking/SecondaryVertexFinder.cc Outdated
Comment thread src/algorithms/tracking/SecondaryVertexFinder.cc
Comment thread src/algorithms/tracking/IterativeVertexFinder.cc Outdated
Comment thread src/algorithms/tracking/IterativeVertexFinder.cc
Comment thread src/algorithms/reco/Helix.cc
Comment thread src/algorithms/reco/Helix.cc Outdated
Comment thread src/algorithms/reco/Helix.cc
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 7, 2026 16:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/algorithms/fardetectors/FarDetectorTransportationPostML.cc:118

  • The prediction-tensor row index is used to select a track, but the code only checks i >= tracks->size() (tracks too small). If the tracks collection is larger than the tensor row count, this silently accepts a size mismatch and may mask upstream wiring/model issues. Consider validating the full 1:1 size match once (and hoisting the nullptr check) before using i-based mapping.
    if (tracks == nullptr) {
      error("No tracks collection provided; cannot set ReconstructedParticle-Track relation");
      throw std::runtime_error("No tracks collection provided");
    }
    if (i >= tracks->size()) {

src/algorithms/tracking/SecondaryVertexFinder.cc:105

  • std::numeric_limits<double>::epsilon() (~2e-16) is effectively zero at the mm scale used here, so the local-position match will almost never succeed unless values are bit-identical. This can prevent associating vertices back to reconstructed particles. Use a physically meaningful tolerance (IterativeVertexFinder uses 1e-4 mm).
          const auto& trkPars = traj.getTrackParameters();
          for (const auto& trkPar : trkPars) {
            double EPSILON = std::numeric_limits<double>::epsilon();
            if (std::abs((trkPar.getLoc().a / edm4eic::unit::mm) -
                         (loc_a / Acts::UnitConstants::mm)) < EPSILON &&

Copilot AI review requested due to automatic review settings August 9, 2026 07:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/algorithms/fardetectors/FarDetectorTransportationPostML.cc:123

  • The ReconstructedParticle–Track relation is set by indexing tracks->at(i), but the code only errors when the prediction tensor has more rows than the tracks collection. If the tracks collection is larger than the tensor row count, the extra tracks will be silently ignored (and downstream relations may be incomplete), even though FarDetectorTransportationPreML builds tensors with shape(0) == tracks->size(). Consider validating that the tensor row count matches tracks->size() exactly before using i as the mapping key.
    if (i >= tracks->size()) {
      error("Prediction tensor row {} has no corresponding track (tracks size={})", i,
            tracks->size());
      throw std::runtime_error("Prediction tensor/track size mismatch");
    }

Copilot AI review requested due to automatic review settings August 20, 2026 12:05
@simonge
simonge requested a review from a team August 20, 2026 12:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/algorithms/fardetectors/FarDetectorTransportationPostML.cc:118

  • The tensor-row → track mapping currently only checks i >= tracks->size(), which catches the case where the prediction tensor has more rows than tracks, but it silently allows the opposite case (tracks size > tensor rows). Given FarDetectorTransportationPreML sizes the feature tensor using inputTracks->size(), a size mismatch in either direction likely indicates a pipeline/config problem and should fail fast. Also, doing the size check once (instead of per-row) improves readability.
    if (tracks == nullptr) {
      error("No tracks collection provided; cannot set ReconstructedParticle-Track relation");
      throw std::runtime_error("No tracks collection provided");
    }
    if (i >= tracks->size()) {

veprbl
veprbl previously approved these changes Aug 20, 2026
Comment thread src/algorithms/fardetectors/FarDetectorTransportationPostML.cc Outdated
Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
Copilot AI review requested due to automatic review settings August 20, 2026 15:56
@simonge
simonge enabled auto-merge August 20, 2026 15:57
@simonge
simonge requested a review from veprbl August 20, 2026 15:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/algorithms/fardetectors/FarDetectorTransportationPostML.cc:118

  • The thrown std::runtime_error message drops the useful context (row index and collection size) that you already compute for the log line. Including the details in the exception makes failures easier to diagnose, especially if exceptions are surfaced without the preceding log message.
    if (i >= tracks->size()) {
      error("Prediction tensor row {} has no corresponding track (tracks size={})", i,
            tracks->size());
      throw std::runtime_error("Prediction tensor/track size mismatch");
    }

src/algorithms/tracking/TracksToParticles.cc:33

  • track_assocs is modeled as optional in TracksToParticles (see TracksToParticles.h), but later in this method it is unconditionally dereferenced (for (auto track_assoc : *track_assocs)), which will crash if the associations collection is not provided. The same applies to optional outputs (part_links, part_assocs) if this algorithm is ever used without them. Please add null/availability guards before dereferencing optional inputs/outputs.
  for (const auto& track : *tracks) {
    auto trajectory = track.getTrajectory();
    if (!trajectory.isAvailable()) {
      continue;
    }

Copilot AI review requested due to automatic review settings September 1, 2026 20:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The new tensor-row-to-track association relies on 1:1 sizing but currently only guards one mismatch direction, which can silently drop tracks or mislead consumers if shapes ever diverge.

Review details

Suppressed comments (1)

src/algorithms/fardetectors/FarDetectorTransportationPostML.cc:120

  • The code associates prediction tensor row i to tracks->at(i), but it only guards the case where there are fewer tracks than prediction rows. If the tensor has fewer rows than the track collection (unexpected but possible if the ONNX output shape changes/mismatches), the extra tracks will be silently ignored while still implying a 1:1 row↔track mapping. Consider enforcing prediction_tensor.getShape(0) == tracks->size() once before attaching tracks, and then unconditionally attaching tracks->at(i).
    if (i >= tracks->size()) {
      error("Prediction tensor row {} has no corresponding track (tracks size={})", i,
            tracks->size());
      throw std::runtime_error("Prediction tensor/track size mismatch");
    }
    particle.addToTracks(tracks->at(i));

  • Files reviewed: 8/8 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Comment on lines +40 to +42
if (!traj.isAvailable()) {
continue;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the long-term vision for this? Are we adding the trajectory for far backward tracks and removing this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll admit I still don't fully understand the layers of tracking objects in the data model. I don't think there is a need to add trajectories and track parameters to the far backwards code. Someone in the future (potentially myself down the line) might refactor to use more centralized acts methods which would presumably create them.

The short term need is for attaching an existing time to the output reconstructed particles. A vertex time would be better but will need some extra work.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Would this be ok to merge as is and continue the discussion as necessary elsewhere?

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

Labels

topic: far-backward Reconstruction related to far backward detectors topic: tracking Relates to tracking reconstruction

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants