feat: compatibility for EDM4hep < 0.99 & >= 0.99
#3921
Conversation
WalkthroughIn this pull request, changes made involve the removal of specific include statements from Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
Component - Examples
Examples/Io/EDM4hep/include/ActsExamples/Io/EDM4hep/EDM4hepUtil.hpp
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
Examples/Io/EDM4hep/src/EDM4hepMeasurementWriter.cpp (1)
58-58: Changed the collection type has been, hmmmm.Correctly aligned with the new EDM4hep structure, this change is. Yet remember you must, that temporary this compatibility layer is. Plan for future cleanup when support for older versions drops, you should.
When the time comes to drop support for EDM4hep <0.99:
- Remove the conditional compilation blocks
- Update all direct TrackerHit3DCollection usages
- Clean up any remaining aliases
Examples/Io/EDM4hep/src/EDM4hepMeasurementReader.cpp (1)
21-31: Wise handling of compatibility, this is! Hmmmm.A elegant solution for version compatibility, you have created. The conditional compilation block, handle different EDM4hep versions it does, yes.
Consider documenting the version compatibility in the header file, help future maintainers it will.
Examples/Io/EDM4hep/include/ActsExamples/Io/EDM4hep/EDM4hepUtil.hpp (1)
27-37: Wise solution for version compatibility, this is. Yes, hmmm.Elegant handling of different EDM4hep versions, you have achieved. Through the Force of type aliases, backward compatibility maintained is. When the time comes to drop support for older versions, clean this shall be.
When support for versions below 0.99 no longer needed is, remove this conditional block you should. Simple cleanup, it will be.
Plugins/EDM4hep/src/EDM4hepUtil.cpp (1)
192-211: Consider runtime version check for future flexibility, you should!In future iterations, runtime version detection mechanism, consider you might. More flexible upgrades and backwards compatibility, it would allow. Less preprocessor magic, better maintainability brings.
Example approach:
namespace Acts::EDM4hepUtil::detail { inline bool isLegacyVersion() { return (EDM4HEP_VERSION_MAJOR < 1 && EDM4HEP_VERSION_MINOR < 99); } template<typename T> edm4hep::MCParticle getParticle(const T& hit) { if (isLegacyVersion()) { return hit.getMCParticle(); } return hit.getParticle(); } }Examples/Io/EDM4hep/src/EDM4hepUtil.cpp (1)
269-280: Elegant refactoring, this is. Yet improve it further, we can!More readable, the covariance matrix construction has become. But const correctness, add we should.
- auto c = [&](std::size_t row, std::size_t col) { + const auto c = [&](std::size_t row, std::size_t col) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
Examples/Io/EDM4hep/include/ActsExamples/Io/EDM4hep/EDM4hepMeasurementWriter.hpp(0 hunks)Examples/Io/EDM4hep/include/ActsExamples/Io/EDM4hep/EDM4hepUtil.hpp(3 hunks)Examples/Io/EDM4hep/src/EDM4hepMeasurementReader.cpp(2 hunks)Examples/Io/EDM4hep/src/EDM4hepMeasurementWriter.cpp(2 hunks)Examples/Io/EDM4hep/src/EDM4hepReader.cpp(2 hunks)Examples/Io/EDM4hep/src/EDM4hepUtil.cpp(6 hunks)Plugins/EDM4hep/include/Acts/Plugins/EDM4hep/EDM4hepUtil.hpp(2 hunks)Plugins/EDM4hep/src/EDM4hepUtil.cpp(2 hunks)
💤 Files with no reviewable changes (1)
- Examples/Io/EDM4hep/include/ActsExamples/Io/EDM4hep/EDM4hepMeasurementWriter.hpp
🔇 Additional comments (14)
Examples/Io/EDM4hep/src/EDM4hepMeasurementWriter.cpp (1)
21-29: Hmmmm, wise handling of compatibility, this is.
Handle different EDM4hep versions elegantly, you do. Proper use of conditional compilation and type aliases, I sense. Yet verify the compatibility across the codebase, we must.
✅ Verification successful
Verified, the compatibility handling is. Consistent across the codebase, it remains.
Through the Force, I sense that all files using TrackerHit3D have proper version compatibility checks in place:
- EDM4hepMeasurementWriter.cpp
- EDM4hepMeasurementReader.cpp
- EDM4hepUtil.hpp
The same pattern of conditional includes and type aliases, they follow. SimTrackerHitCollection references, a different entity they are, untouched they should remain.
Balanced and harmonious, the implementation is. No further changes, we require.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining direct usage of TrackerHitCollection that might need updating
# Search for direct usage of TrackerHitCollection
rg "TrackerHitCollection" --type cpp
# Search for potential missed includes of TrackerHit3DCollection
rg "TrackerHit3D" --type cpp
Length of output: 3463
Examples/Io/EDM4hep/src/EDM4hepMeasurementReader.cpp (2)
73-73: Updated collection type, matches the new way it does.
Correctly using TrackerHit3DCollection now, you are. But verify the collection name in the frame, we should.
Check if "ActsTrackerHitsRaw" collection name remains consistent:
✅ Verification successful
Verified, the collection name consistency is, hmm!
Found perfect alignment between writer and reader, I have. In both locations, "ActsTrackerHitsRaw" the collection name remains:
- In EDM4hepMeasurementWriter.cpp: Writing to "ActsTrackerHitsRaw" it does
- In EDM4hepMeasurementReader.cpp: Reading from "ActsTrackerHitsRaw" it does
Consistent the naming is, young padawan. No changes needed, there are.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for collection name usage
rg "ActsTrackerHitsRaw" --type cpp -A 2 -B 2
Length of output: 985
28-29: Clever type aliases, you have made.
Bridge the old and new worlds, these aliases do. When support for older versions drops, easier the transition will be, hmmmm.
Verify the usage of these aliases throughout the codebase, we must:
✅ Verification successful
Verified, the usage of type aliases is, throughout the codebase.
Hmmmm, clear evidence we have found. The type aliases TrackerHit3D and TrackerHit3DCollection bridge consistently between old and new EDM4hep types they do. Used in multiple files they are:
Examples/Io/EDM4hep/src/EDM4hepMeasurementReader.cppExamples/Io/EDM4hep/src/EDM4hepMeasurementWriter.cppExamples/Io/EDM4hep/include/ActsExamples/Io/EDM4hep/EDM4hepUtil.hpp
Conditional compilation with __has_include guards, protect the transitions they do. Wise decision this is, for backward compatibility it maintains.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of both old and new types
echo "=== Searching for TrackerHit usage ==="
rg "TrackerHit[^3]" --type cpp
echo "=== Searching for TrackerHit3D usage ==="
rg "TrackerHit3D" --type cpp
Length of output: 7898
Examples/Io/EDM4hep/include/ActsExamples/Io/EDM4hep/EDM4hepUtil.hpp (2)
19-26: Organized well, these headers are. Hmmmm.
Proper use of angle brackets for external dependencies, I sense. Clear and consistent, the include structure is.
104-104: Verify the usage of TrackerHit3D throughout the codebase, we must.
Changed these parameter types are, but ensure we must that all callers updated have been.
Also applies to: 120-120
✅ Verification successful
Consistent with the Force, these changes are.
Hmmmm. Through the code I have looked, and clear it has become:
- The
TrackerHitreferences that remain, in compatibility code they exist, or in different contexts they dwell - The
readMeasurementandwriteMeasurementfunctions, properly updated they are, withTrackerHit3Dparameters they use - The callers of these functions, in harmony they operate, with the new parameter types they do
No disturbance in the Force I sense. The parameter type changes, properly implemented they are, throughout the codebase they flow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of old TrackerHit types
rg -l "TrackerHitCollection|TrackerHit[^3]" --type cpp
# Search for calls to these functions
ast-grep --pattern 'readMeasurement($$$)'
ast-grep --pattern 'writeMeasurement($$$)'
Length of output: 663
Script:
#!/bin/bash
# Check the content of files that still use TrackerHit to see the context
rg "TrackerHit[^3]" Examples/Io/EDM4hep/src/EDM4hepUtil.cpp -C 2
rg "TrackerHit[^3]" Examples/Io/EDM4hep/src/EDM4hepMeasurementReader.cpp -C 2
rg "TrackerHit[^3]" Examples/Io/EDM4hep/src/EDM4hepMeasurementWriter.cpp -C 2
# Look for function declarations/implementations
ast-grep --pattern 'readMeasurement($_, $_, $_)'
ast-grep --pattern 'writeMeasurement($_, $_, $_)'
Length of output: 2576
Plugins/EDM4hep/src/EDM4hepUtil.cpp (2)
21-25: Wise additions to the headers, these are!
Properly organized, the new includes are. Version header for conditional compilation, essential it is. Match the requirements for TrackerHit3D compatibility, they do.
192-211: Hmmmm... Handle version differences wisely, you do!
Elegant solution for API changes, this is. Both old and new versions of EDM4hep, support you must. Proper separation between APIs, maintained you have.
Verify the version check condition, we must:
✅ Verification successful
Consistent version handling across the codebase, I sense
Through the Force, searched the entire codebase, I have. Clear findings, these are:
- Only one version check for EDM4HEP exists, properly guarding the API changes
- Old API (
getMCParticle/setMCParticle) appears only within the version-guarded block - New API (
getParticle/setParticle) usage outside EDM4hepUtil.cpp relates to different functionality in Fatras and other components - Clean separation between old and new APIs maintained, it is
Hmmmm... Wise implementation this is. No conflicts or inconsistencies in the version handling, there are.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify EDM4hep version handling across the codebase
# Search for other version checks to ensure consistency
rg -A 2 "EDM4HEP_VERSION" --type cpp
# Search for any remaining old API usage
rg -A 2 "getMCParticle|setMCParticle" --type cpp
# Search for any new API usage
rg -A 2 "getParticle\(|setParticle\(" --type cpp
Length of output: 9406
Plugins/EDM4hep/include/Acts/Plugins/EDM4hep/EDM4hepUtil.hpp (2)
29-32: Hmmmm, wise additions these includes are.
Properly aligned with the Force, these new headers are. Support the transition between EDM4hep versions, they do.
66-70: Handle the version differences with grace, we must.
Compatibility functions these are, bridging the gap between EDM4hep versions they do. Wise decision to abstract version-specific changes through utility functions, this is.
Yet verify their usage across the codebase, we should.
✅ Verification successful
Properly handled, the version differences are, hmmmm.
Clear it is, that version-specific implementation through preprocessor directives exists. Wisely structured, the code is:
- For EDM4hep >= 0.99:
getParticle()andsetParticle()used - For older versions:
getMCParticle()andsetMCParticle()maintained
No further verification needed, as the Force shows us the compatibility layer works in harmony with both old and new versions it does. Through preprocessor guards, safely managed this transition is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of deprecated getMCParticle to ensure proper migration
rg "getMCParticle" --type cpp
# Search for usage of new getParticle function
rg "getParticle.*SimTrackerHit" --type cpp
# Search for usage of new setParticle function
rg "setParticle.*MutableSimTrackerHit" --type cpp
Length of output: 823
Script:
#!/bin/bash
# Search for actual usage of these utility functions in the codebase
rg -B2 -A2 "getParticle\(.*SimTrackerHit" --type cpp
rg -B2 -A2 "setParticle\(.*MutableSimTrackerHit" --type cpp
# Check implementation details in EDM4hepUtil.cpp
cat Plugins/EDM4hep/src/EDM4hepUtil.cpp
# Look for version-specific code or preprocessor directives
rg "#if.*EDM4HEP" --type cpp
rg "#ifdef.*EDM4HEP" --type cpp
rg "#ifndef.*EDM4HEP" --type cpp
Length of output: 10582
Examples/Io/EDM4hep/src/EDM4hepUtil.cpp (3)
120-120: Strong with the Force, this change is!
Symmetry with getParticle() function, this maintains. Consistent approach to particle handling, it provides.
73-76: Hmmmm, wise changes these are, approve them I do!
Compatibility with both old and new EDM4hep versions, this change ensures. Through getParticle() function, safely accessed the particle data is.
✅ Verification successful
Safe and sound, these changes are!
Inside preprocessor guards, the remaining getMCParticle() call lies, hmm. A compatibility layer this creates, allowing the code to work with different EDM4hep versions it does. Wise decision this is, as backwards compatibility it maintains while forward-looking changes it embraces.
The Force is strong with this code change, as:
- In new version path, getParticle() function it uses
- In older version path, getMCParticle() safely preserved it is
- Through preprocessor guards, both paths cleanly separated they are
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify no direct getMCParticle calls remain
rg "getMCParticle" --type cpp
Length of output: 95
Script:
#!/bin/bash
# Check the context of the remaining getMCParticle call
rg "getMCParticle" --type cpp -B 5 -A 5
# Find the definition of getParticle to understand the migration pattern
ast-grep --pattern 'getParticle($$$) {
$$$
}'
Length of output: 820
Line range hint 150-175: Incomplete, this implementation seems. Clarification, we need!
About the cell information accessibility, uncertain we are. Document the implications of ignoring cluster parameters, you must. Future maintainers, confused they will be.
Examples/Io/EDM4hep/src/EDM4hepReader.cpp (2)
13-13: Hmmmm, wise addition this include is!
Align with PR objectives, this change does. Support for EDM4hep utility functions, it provides.
309-311: Verify the impact of getParticle usage, we must!
Changed from direct access to getMCParticle to using Acts::EDM4hepUtil::getParticle, this code has. Ensure backward compatibility with older EDM4hep versions, we should.
Run this script, we shall:
✅ Verification successful
Safe, this change is! Backward compatibility, it maintains!
Examined the implementation, I have. Clear it is that Acts::EDM4hepUtil::getParticle serves as a compatibility layer, hmm! In the EDM4hepUtil implementation, see we do that getMCParticle is still used internally:
// In EDM4hepUtil.cpp
Plugins/EDM4hep/src/EDM4hepUtil.cpp: return hit.getMCParticle();A wrapper function this is, maintaining backward compatibility it does. No impact on older EDM4hep versions, there will be. Through the Force, seen I have that this change improves code organization without breaking existing functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other direct uses of getMCParticle that might need updating
# Search for direct getMCParticle calls
echo "=== Direct getMCParticle calls ==="
rg "getMCParticle\(" --type cpp
# Search for EDM4hepUtil::getParticle usage
echo -e "\n=== EDM4hepUtil::getParticle usage ==="
rg "EDM4hepUtil::getParticle\(" --type cpp
Length of output: 531
Script:
#!/bin/bash
# Investigate the EDM4hepUtil implementation and its usage, we must
# First, examine the EDM4hepUtil implementation
echo "=== EDM4hepUtil implementation ==="
cat Examples/Io/EDM4hep/src/EDM4hepUtil.cpp
# Then, check for any version-dependent code or compatibility checks
echo -e "\n=== Version checks or compatibility handling ==="
rg -i "version|compat" Examples/Io/EDM4hep/
# Finally, examine the header file for any relevant documentation
echo -e "\n=== EDM4hepUtil header ==="
fd EDM4hepUtil.hpp --exec cat {}
Length of output: 25805
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Plugins/EDM4hep/include/Acts/Plugins/EDM4hep/TrackerHitCompatibility.hpp (1)
11-22: Wise approach to version compatibility, this is! Hmmmm.A elegant solution for handling different EDM4hep versions, you have created. The
__has_includemacro, a powerful tool it is, detecting the presence of new headers without runtime overhead.Consider documenting the version compatibility strategy in the codebase's documentation, help future maintainers it will. When support for older versions is dropped, easier to update this will be.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Examples/Io/EDM4hep/src/EDM4hepMeasurementReader.cpp(2 hunks)Examples/Io/EDM4hep/src/EDM4hepMeasurementWriter.cpp(2 hunks)Plugins/EDM4hep/include/Acts/Plugins/EDM4hep/TrackerHitCompatibility.hpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Examples/Io/EDM4hep/src/EDM4hepMeasurementReader.cpp
- Examples/Io/EDM4hep/src/EDM4hepMeasurementWriter.cpp
🔇 Additional comments (1)
Plugins/EDM4hep/include/Acts/Plugins/EDM4hep/TrackerHitCompatibility.hpp (1)
18-21: Clean type aliasing in namespace, I sense. Strong with the Force, this code is!
In the edm4hep namespace, properly scoped the aliases are. Prevent naming conflicts, this will. A path to maintainable code, you have chosen.
Verify the usage of these aliases throughout the codebase, we must:
✅ Verification successful
Consistent usage of aliases throughout the codebase, I observe. Proceed with confidence, we can.
Through the Force, clear patterns I see:
- In both
Plugins/EDM4hepandExamples/Io/EDM4hep, properly defined and used, the aliases are - Consistent usage of
TrackerHit3DandTrackerHit3DCollectionin alignment with their base typesTrackerHitandTrackerHitCollection, I detect - No conflicts or inconsistencies in the type usage across the codebase, there are
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of both old and new types
echo "=== Searching for TrackerHit3D usage ==="
rg "TrackerHit3D[^Collection]" --type cpp
echo "=== Searching for TrackerHit3DCollection usage ==="
rg "TrackerHit3DCollection" --type cpp
echo "=== Searching for old TrackerHit usage ==="
rg "TrackerHit[^3D]" --type cpp
Length of output: 8039
paulgessinger
changed the title
0.99 & >= 0.99
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Plugins/EDM4hep/src/EDM4hepUtil.cpp (1)
197-213: Document the ways of the Force, we should.A comment explaining the version differences and migration path, helpful it would be. Future Padawans, grateful they will be.
Add documentation above the functions:
+// These functions provide version-specific implementations for particle handling +// For EDM4hep >= 0.99: Uses getParticle/setParticle +// For EDM4hep < 0.99: Uses getMCParticle/setMCParticle +// This abstraction allows for seamless version compatibility
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Plugins/EDM4hep/src/EDM4hepUtil.cpp(2 hunks)
🔇 Additional comments (2)
Plugins/EDM4hep/src/EDM4hepUtil.cpp (2)
21-28: Wise changes in the Force of includes and namespaces, I sense.
Properly organized, the new headers are. Clear path to EDM4hep version compatibility, they provide. Good structure, the namespace changes bring.
195-214: Hmmmm... Version compatibility, handle with care we must.
Wise approach to handle both old and new EDM4hep versions, this is. But verify the version check across the codebase, we should.
✅ Verification successful
Consistent version checks across the codebase, they are. Safe to proceed, it is.
Through the Force, searched the entire codebase I have. Only one EDM4hep version check exists, precisely in the file under review. Other occurrences of EDM4HEP_VERSION only in CMake configuration, they are.
The version-dependent particle handling functions getParticle and setParticle properly encapsulated in EDM4hepUtil, they are. No direct calls to getMCParticle or setMCParticle elsewhere in codebase found, hmmmm. All interactions through our utility functions, they flow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other EDM4hep version checks to ensure consistency
# Search for other version checks
echo "=== Searching for EDM4hep version checks ==="
rg "EDM4HEP_VERSION" -A 2 -B 2
# Search for potential uses of getMCParticle/getParticle
echo "=== Searching for particle getter/setter usage ==="
rg "(getMCParticle|getParticle|setMCParticle|setParticle)" -A 2 -B 2
Length of output: 23743
Examples/Io/EDM4hep/include/ActsExamples/Io/EDM4hep/EDM4hepUtil.hpp
Outdated
Show resolved
Hide resolved
|
This adds jobs for LCG 106a to the GitLab CI. I'm bumping ODD to the [`v4.0.3`](https://gitlab.cern.ch/acts/OpenDataDetector/-/tags/v4.0.3) where I cherry picked a tiny code fix that was breaking the build here. Blocked by: - #3921 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new CI/CD job `lcg_106a` for enhanced build configurations. - **Improvements** - Simplified the `lcg_105` job by directly setting `LCG_PLATFORM` for the `alma9` OS. - **Updates** - Updated the commit reference for the `OpenDataDetector` subproject to the latest version. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This updates our code so it compiles with both EDM4hep versions below
0.99and above/equal to0.99.Breaking changes are:
TrackerHitbecomesTrackerHit3D.SimTrackerHit::getMCParticlebecomesSimTrackerHit::getParticle, with the original name being deprecated.I'm aliasing the new name
TrackerHit3DandTrackerHit3DCollectionto the old types, so that when we drop support for <0.99versions, they're already correct.Summary by CodeRabbit
New Features
SimTrackerHitobjects.Bug Fixes
EDM4hepReaderfor missing particles.Refactor
EDM4hepMeasurementWriterandEDM4hepMeasurementReaderto accommodate new data structures.Documentation