Skip to content

fix: Remove redundant EDM4eic version guards for minimum 8.7 - #2838

Merged
wdconinc merged 4 commits into
mainfrom
copilot/fix-edm4eic-minimum-version-guard
Aug 12, 2026
Merged

fix: Remove redundant EDM4eic version guards for minimum 8.7#2838
wdconinc merged 4 commits into
mainfrom
copilot/fix-edm4eic-minimum-version-guard

Conversation

Copilot AI commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Removes version guard checks for EDM4eic >= 8.7 across the codebase. The minimum required version is already set to 8.7 in CMakeLists.txt, making these guards always true and unnecessary.

Changes

  • src/algorithms/digi/CALOROCDigitization.cc - Removed guard wrapping entire implementation
  • src/algorithms/tracking/TrackSeeding.cc - Removed guard around setQuality() call
  • src/detectors/BEMC/BEMC.cc - Removed 2 guards (include and factory registration)
  • src/services/io/podio/JEventProcessorPODIO.cc - Removed guard around CALOROC collection names
  • src/tests/algorithms_test/digi_CALOROCDigitization.cc - Removed guard wrapping test

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

Eliminates technical debt by removing always-true preprocessor guards, improving code clarity and maintainability.

What is the urgency of this PR?

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

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue #__)
  • Optimization (issue #__)
  • Updated parameters, constants (issue #__)
  • Updated documentation
  • other: __

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.

Copilot AI changed the title [WIP] Fix guard related to EDM4eic minimum version Remove redundant EDM4eic version guards for minimum 8.7 Aug 6, 2026
Copilot AI requested a review from wdconinc August 6, 2026 20:57
@wdconinc
wdconinc marked this pull request as ready for review August 6, 2026 21:05
Copilot AI lite review requested due to automatic review settings August 6, 2026 21:05

This comment was marked as low quality.

@wdconinc wdconinc closed this Aug 7, 2026
@wdconinc wdconinc reopened this Aug 7, 2026
@github-actions github-actions Bot added topic: calorimetry relates to calorimetry topic: tracking Relates to tracking reconstruction topic: barrel topic: digitization labels Aug 7, 2026
@wdconinc wdconinc changed the title Remove redundant EDM4eic version guards for minimum 8.7 fix: Remove redundant EDM4eic version guards for minimum 8.7 Aug 7, 2026
@wdconinc
wdconinc enabled auto-merge August 7, 2026 13:32
veprbl
veprbl previously approved these changes Aug 7, 2026

@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/tests/algorithms_test/digi_CALOROCDigitization.cc Outdated
Comment thread src/algorithms/tracking/TrackSeeding.cc Outdated
Comment thread src/algorithms/tracking/TrackSeeding.cc Outdated
Comment thread src/algorithms/tracking/TrackSeeding.cc Outdated
wdconinc pushed a commit that referenced this pull request Aug 7, 2026
…2840)

This PR applies the include-what-you-use fixes as suggested by
https://github.com/eic/EICrecon/actions/runs/31183049771.
Please merge this PR into the branch
`copilot/fix-edm4eic-minimum-version-guard`
to resolve failures in PR #2838.

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>
Copilot AI review requested due to automatic review settings August 7, 2026 17:04

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 5 out of 5 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings August 7, 2026 17:08

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 5 out of 5 changed files in this pull request and generated no new comments.

wdconinc pushed a commit that referenced this pull request Aug 10, 2026
…2840)

This PR applies the include-what-you-use fixes as suggested by
https://github.com/eic/EICrecon/actions/runs/31183049771.
Please merge this PR into the branch
`copilot/fix-edm4eic-minimum-version-guard`
to resolve failures in PR #2838.

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>
Copilot AI review requested due to automatic review settings August 10, 2026 22:52
@wdconinc
wdconinc force-pushed the copilot/fix-edm4eic-minimum-version-guard branch from 4db25dc to bd40dcb Compare August 10, 2026 22:52

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 5 out of 5 changed files in this pull request and generated no new comments.

Copilot AI and others added 3 commits August 10, 2026 21:52
Since EDM4eic >= 8.7 is the minimum required version (CMakeLists.txt:63),
all version guards checking for >= 8.7 or > 8.5 are always true and can
be safely removed to simplify the codebase.

Modified files:
- src/algorithms/digi/CALOROCDigitization.cc: Removed guard wrapping entire implementation
- src/algorithms/tracking/TrackSeeding.cc: Removed guard around setQuality call
- src/detectors/BEMC/BEMC.cc: Removed 2 guards (include + factory registration)
- src/services/io/podio/JEventProcessorPODIO.cc: Removed guard around collection names
- src/tests/algorithms_test/digi_CALOROCDigitization.cc: Removed guard wrapping entire test"

Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
…2840)

This PR applies the include-what-you-use fixes as suggested by
https://github.com/eic/EICrecon/actions/runs/31183049771.
Please merge this PR into the branch
`copilot/fix-edm4eic-minimum-version-guard`
to resolve failures in PR #2838.

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>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Wouter Deconinck <wdconinc@gmail.com>
Copilot AI review requested due to automatic review settings August 11, 2026 02:52
@wdconinc
wdconinc force-pushed the copilot/fix-edm4eic-minimum-version-guard branch from bd40dcb to 896a3b3 Compare August 11, 2026 02:52

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 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tests/algorithms_test/digi_CALOROCDigitization.cc:6

  • This file still uses unqualified fixed-width types (e.g. uint16_t) later on. <cstdint> only guarantees these typedefs in std::, while the global aliases are not guaranteed, which can hurt portability. Easiest fix is to keep the previous C header include (<stdint.h>), or alternatively switch the later usage to std::uint16_t.
#include <cstdint>

@wdconinc

Copy link
Copy Markdown
Contributor

Conflict resolved; needs new approval.

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 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tests/algorithms_test/digi_CALOROCDigitization.cc:9

  • <cstdint> only guarantees fixed-width integer typedefs in the std:: namespace. This file still uses uint16_t unqualified later, which can be a compile error on implementations that don’t also provide these typedefs in the global namespace. Prefer std::uint16_t, or add a using declaration after the includes.
#include <cstdint>
#include <cstddef>
#include <memory>

@ruse-traveler ruse-traveler 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.

Thanks for the fix, @wdconinc! Looks good to me!

I am noticing some odd diffs in the capybara report (e.g. in the DRICH theta-phi photons and the BIC N combined pulses, attached here). But given the changes (and that all of them seem to be in multithreading comparisons), I suspect these are unrelated...

Image Image

@wdconinc
wdconinc added this pull request to the merge queue Aug 12, 2026
@wdconinc

Copy link
Copy Markdown
Contributor

Thanks for the fix, @wdconinc! Looks good to me!

I am noticing some odd diffs in the capybara report (e.g. in the DRICH theta-phi photons and the BIC N combined pulses, attached here). But given the changes (and that all of them seem to be in multithreading comparisons), I suspect these are unrelated...

_DRICHGasIrtCherenkovParticleID_thetaPhiPhotons.b has always been irreproducible. It's a problem in (at this point) IRT1, so the priority on fixing it is low (I already spent a few hours on it and found no immediate reasons). I suspect it's going to be a problem in IRT2 as well, but we have not been able to assess that. IRT1 (and IRT2, tbh) are not written with multi-threaded reproducibility in mind.

EcalBarrelScFiNCombinedPulses were just removed in ef20cce, so that's a TOCTOU issue.

Merged via the queue into main with commit 4412355 Aug 12, 2026
164 checks passed
@wdconinc
wdconinc deleted the copilot/fix-edm4eic-minimum-version-guard branch August 12, 2026 15:11
@ruse-traveler

Copy link
Copy Markdown
Contributor

_DRICHGasIrtCherenkovParticleID_thetaPhiPhotons.b has always been irreproducible. It's a problem in (at this point) IRT1, so the priority on fixing it is low (I already spent a few hours on it and found no immediate reasons). I suspect it's going to be a problem in IRT2 as well, but we have not been able to assess that. IRT1 (and IRT2, tbh) are not written with multi-threaded reproducibility in mind.

EcalBarrelScFiNCombinedPulses were just removed in ef20cce, so that's a TOCTOU issue

Ahhhh I see!

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

Labels

topic: barrel topic: calorimetry relates to calorimetry topic: digitization topic: tracking Relates to tracking reconstruction

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Didn't we bump the minimum EDM4eic to 8.7 already? What's this guard doing here?

6 participants