fix: remove *Associations from PODIO output collections

[wdconinc]

Dependencies

• fix(pid): DRICH IRT link migration #2875

Description

Removes 60 unused *Associations entries from the default output collections list in JEventProcessorPODIO.cc. These association collections are no longer needed in the standard PODIO output, reducing file size and output verbosity. For each of the removed *Associations, the *Links are written.

Also, modifies IrtCherenkovParticleID algorithm/factory to use links instead of associations to avoid dangling references.

Changes

• Removed all *Associations collection entries from output_collections vector
• Adds *Links collection outputs to IrtCherenkovParticleID algorithm/factory

This change affects the default PODIO output files generated by EICrecon, excluding these association collections unless explicitly requested via configuration.

Impact

• Smaller output ROOT files by default
• Faster file I/O operations
• Users can still enable specific associations via podio:output_collections parameter if needed

[Copilot]

🟡 Not ready to approve

The implementation and PR description are inconsistent about which *Associations collections are still kept by default, and this needs to be clarified or corrected.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR trims the default PODIO output collection include-list in JEventProcessorPODIO.cc by removing many *Associations collections (while keeping the corresponding *Links collections), aiming to reduce default output size and verbosity.

Changes:

• Removed numerous *Associations entries from the default output_collections list.
• Kept *Links entries alongside the corresponding reconstructed objects.
• Left MCScatteredElectronAssociations / MCNonScatteredElectronAssociations (and also ReconstructedChargedParticleAssociations) in the default list.

File summaries

File	Description
src/services/io/podio/JEventProcessorPODIO.cc	Removes many *Associations collections from the default PODIO output collection list to reduce persisted output volume.

Review details

Suppressed comments (1)

src/services/io/podio/JEventProcessorPODIO.cc:216

• The PR description says all *Associations entries were removed except the two MC*ElectronAssociations, but ReconstructedChargedParticleAssociations is still in the default output list. Either update the PR description to reflect this exception, or remove this entry if it is also intended to be excluded by default.

      "ReconstructedChargedParticles",
      "ReconstructedChargedParticleLinks",
      "ReconstructedChargedParticleAssociations",
      "MCScatteredElectronAssociations",    // Remove if/when used internally
      "MCNonScatteredElectronAssociations", // Remove if/when used internally

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

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟢 Ready to approve

The changes are focused and consistent with the stated goal, with only a minor wording clarification suggested in an inline comment.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

src/services/io/podio/JEventProcessorPODIO.cc:217

• The inline comments for the temporary MC*Electron{Links,Associations} entries read "Remove if/when used internally", which is ambiguous and appears inverted (if they are used internally, they should not be removed). Reword to clarify they can be removed once they are no longer needed internally / after migration.

      "MCScatteredElectronLinks",           // Remove if/when used internally
      "MCScatteredElectronAssociations",    // Remove if/when used internally
      "MCNonScatteredElectronLinks",        // Remove if/when used internally
      "MCNonScatteredElectronAssociations", // Remove if/when used internally

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

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The default output now writes subset MC*ElectronAssociations collections without writing their parent ReconstructedChargedParticleAssociations, which can produce dangling collectionID references in the output.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

src/global/reco/reco.cc:17

• The new #include <podio/detail/Link.h> and #include <deque> are unused in this translation unit, increasing compile time and risking unused-include warnings under stricter builds/clang-tidy.

#include <edm4hep/MCParticle.h>
#include <podio/detail/Link.h>
#include <deque>
#include <map>

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

Pull request overview

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

Suppressed comments (3)

src/algorithms/pid/IrtCherenkovParticleID.cc:29

• Avoid including PODIO internal headers from podio/detail/* here. These headers are not part of the stable public API and are currently unused in this file, which can cause brittle builds across PODIO versions.

#include <podio/ObjectID.h>
#include <podio/RelationRange.h>
#include <podio/detail/LinkCollectionImpl.h>
#include <podio/detail/LinkCollectionIterator.h>
#include <algorithm>

src/algorithms/pid/IrtCherenkovParticleID.cc:147

• The trace message says "number of raw sensor hit" (singular) but logs a count. Update wording to plural (and mention links explicitly) to avoid confusing log output.

  trace("number of raw sensor hits: {}", in_raw_hits->size());
  trace("number of raw sensor hit with associated photons: {}", in_hit_links->size());

src/algorithms/pid/IrtCherenkovParticleID.cc:234

• This comment still says the MC photon lookup loops over in_hit_assocs, but the code now loops over in_hit_links. Updating the comment avoids misleading future readers when debugging cheat-mode behavior.

        // get MC photon(s), typically only used by cheat modes or trace logging
        // - loop over `in_hit_assocs`, searching for the matching hit association
        // - will not exist for noise hits
        edm4hep::MCParticle mc_photon;
        bool mc_photon_found = false;
        if (m_cfg.cheatPhotonVertex || m_cfg.cheatTrueRadiator) {
          for (const auto& hit_link : *in_hit_links) {
            if (hit_link.getFrom().isAvailable()) {

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/services/io/podio/JEventProcessorPODIO.cc:216

• PR description/title says all *Associations outputs were removed from the default PODIO output list, but there are still association collections present elsewhere in output_collections (e.g. DRICHRawHitsAssociations, ReconstructedChargedParticleAssociations, MCScatteredElectronAssociations). Please align the PR description with the actual behavior (document exceptions), or remove the remaining association outputs if they are truly no longer needed.

      "ReconstructedParticles",
      "ReconstructedParticleLinks",
      "ReconstructedTruthSeededChargedParticles",
      "ReconstructedTruthSeededChargedParticleLinks",
      "ReconstructedChargedRealPIDParticles",

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/services/io/podio/JEventProcessorPODIO.cc:71

• The PR title/description say all *Associations were removed from the default PODIO output list and that IrtCherenkovParticleID was migrated to links. However, JEventProcessorPODIO.cc still contains several *Associations outputs (e.g. DRICHRawHitsAssociations, MCScatteredElectronAssociations, MCNonScatteredElectronAssociations), and IrtCherenkovParticleID still persists RawHitAssociations relations via addToRawHitAssociations(...) (see src/algorithms/pid/IrtCherenkovParticleID.cc:470-473). Please either update the PR description/title to reflect what is actually changed, or complete the link migration/removal for the remaining association outputs so defaults are consistent.

      "CentralTrackerTruthSeeds",
      "CentralTrackingRecHits",
      "CentralTrackingRawHitLinks",
      "CentralTrackSeeds",
      "CentralTrackSeedParameters",

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/services/io/podio/JEventProcessorPODIO.cc:215

• Although many *Associations entries are removed from the default output_collections, the list still includes some association collections (e.g. ReconstructedChargedParticleAssociations, MCScatteredElectronAssociations, MCNonScatteredElectronAssociations, and also DRICHRawHitsAssociations earlier). This conflicts with the PR description/title stating that *Associations entries are removed from the default PODIO output. Please either remove the remaining association collections as well, or update the PR description (and/or add an inline comment) to clarify which association collections intentionally remain and why.

      "ReconstructedParticles",
      "ReconstructedParticleLinks",
      "ReconstructedTruthSeededChargedParticles",
      "ReconstructedTruthSeededChargedParticleLinks",