refactor!: Move GBTS to experimental namespace #4048
Conversation
WalkthroughHmm, a significant transformation, this pull request represents. Namespace migration from Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
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 - Core
Rosie-Hasan
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
Core/src/TrackFinding/RoiDescriptor.cpp (1)
Incomplete, the namespace migration appears to be, young Padawan!
In the Force, a disturbance I sense:
- Header file
Core/include/Acts/TrackFinding/RoiDescriptor.hppin old location remains, while implementation toActs::Experimentalhas moved- Consistency in the codebase, this breaks
- Move the header file to
Core/include/Acts/Experimental/TrackFinding/RoiDescriptor.hpp, you must🔗 Analysis chain
Line range hint
15-43: Correct, the namespace migration is. Hmmmm.To Acts::Experimental namespace, the RoiDescriptor has moved. Breaking change this is, but aligned with PR objectives it stands.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of Acts::RoiDescriptor that needs updating rg "Acts::RoiDescriptor" --type cppLength of output: 35
Script:
#!/bin/bash # Search for any RoiDescriptor usage, includes, and namespace patterns echo "=== Searching for RoiDescriptor usage ===" rg "RoiDescriptor" --type cpp -g '!RoiDescriptor.cpp' echo -e "\n=== Checking include statements ===" rg "#include.*RoiDescriptor" --type cpp echo -e "\n=== Checking namespace patterns ===" rg "namespace.*RoiDescriptor|using.*RoiDescriptor" --type cppLength of output: 2826
Core/include/Acts/Seeding/GbtsBase.hpp (1)
Line range hint
13-17: Replace macros with modern C++ constants, we must.Magic numbers in macros, a path to the dark side they are. Consider:
-#define MAX_SILICON_LAYER_NUM 19 -#define OffsetEndcapPixels 7 -#define OffsetBarrelSCT 3 -#define OffsetEndcapSCT 10 +namespace Acts::Experimental { +constexpr int MAX_SILICON_LAYER_NUM = 19; +constexpr int OffsetEndcapPixels = 7; +constexpr int OffsetBarrelSCT = 3; +constexpr int OffsetEndcapSCT = 10; +}
🧹 Nitpick comments (4)
Examples/Algorithms/TrackFinding/src/GbtsSeedingAlgorithm.cpp (1)
110-110: Commented-out code remains.
Remove or keep for future reference, decide you must.Core/include/Acts/Seeding/SeedFinderGbts.ipp (1)
430-430: New vector instantiation in loop, done carefully.
One memory reallocation you do, though performance risk minimal.Core/include/Acts/Seeding/SeedFinderGbtsConfig.hpp (1)
Line range hint
23-97: Reorganize the configuration structure, we should. Hmmmm.Many magic numbers and loose configuration parameters, I sense. Consider:
- Group related parameters into sub-structures
- Use constexpr for compile-time constants
- Add validation for parameter ranges
Example reorganization:
template <typename SpacePoint> struct SeedFinderGbtsConfig { + struct GeometryConfig { + float minDeltaRadius = 2.0; + float maxOuterRadius = 550.0; + std::vector<TrigInDetSiLayer> layerGeometry; + bool useEtaBinning = true; + }; + + struct CutConfig { + float dphi_max = 0.012; + float dcurv_max = 0.001; + float tau_ratio_max = 0.007; + float d0_max = 4.0; + }; + + GeometryConfig geometry; + CutConfig cuts;Core/include/Acts/Seeding/GbtsDataStorage.hpp (1)
21-21: Complex data structures, moved to experimental realm they are.Properly encapsulated in
Acts::Experimentalnamespace, these data structures are. No functional changes observed, only namespace migration performed. A good candidate for experimental features, this complexity is.Consider breaking down complex data structures into smaller, more manageable components in future iterations, we should.
Also applies to: 289-289
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
Core/include/Acts/Seeding/GbtsBase.hpp(1 hunks)Core/include/Acts/Seeding/GbtsDataStorage.hpp(2 hunks)Core/include/Acts/Seeding/GbtsGeometry.hpp(4 hunks)Core/include/Acts/Seeding/GbtsTrackingFilter.hpp(7 hunks)Core/include/Acts/Seeding/SeedFinderGbts.hpp(4 hunks)Core/include/Acts/Seeding/SeedFinderGbts.ipp(14 hunks)Core/include/Acts/Seeding/SeedFinderGbtsConfig.hpp(2 hunks)Core/include/Acts/TrackFinding/GbtsConnector.hpp(3 hunks)Core/include/Acts/TrackFinding/RoiDescriptor.hpp(2 hunks)Core/src/TrackFinding/GbtsConnector.cpp(2 hunks)Core/src/TrackFinding/RoiDescriptor.cpp(2 hunks)Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/GbtsSeedingAlgorithm.hpp(2 hunks)Examples/Algorithms/TrackFinding/src/GbtsSeedingAlgorithm.cpp(8 hunks)Examples/Python/src/TrackFinding.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- Core/include/Acts/TrackFinding/RoiDescriptor.hpp
- Core/src/TrackFinding/GbtsConnector.cpp
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: merge-sentinel
- GitHub Check: unused_files
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: macos
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: docs
- GitHub Check: build_debug
- GitHub Check: linux_ubuntu
🔇 Additional comments (41)
Examples/Algorithms/TrackFinding/src/GbtsSeedingAlgorithm.cpp (11)
26-32: Rename to experimental namespace, consistent it is.
No functional changes I sense. Continue, you must.
73-74: Connector creation in Experimental space, see I do.
Implementation correct, disrupt the Force it does not.
76-76: Geometry usage in new namespace, wise move this is.
Maintain clarity in parameter passing, you should.
85-85: Space point vector creation, easy to grasp it is.
Naming consistent, you keep.
98-98: Instantiation of SeedFinderGbts, good synergy with geometry.
No code smell, no worry.
107-108: RoiDescriptor creation, neat and straightforward it looks.
Parameters updated, correct they seem.
152-152: Return type updated to Experimental SP, consistent it is.
Yes, properly aligned with the new namespace, it now is.
157-158: Space points vector, lines these are.
Pre-allocation usage, decent performance yields.
233-233: Return type changed to Experimental TrigInDetSiLayer, see I do.
Behave well, it should, so proceed.
235-235: Layer numbering vector, now in Experimental space.
All is aligned, powerful synergy you have.
333-334: Constructing new TrigInDetSiLayer, yes.
Parameters included, handle carefully you will.Core/include/Acts/Seeding/SeedFinderGbts.ipp (18)
31-31: Namespace declaration to Experimental, big step.
Stable, the approach remains.
62-63: RoiDescriptor usage from Experimental, powerful adaptation.
No illusions, correct it looks.
79-79: Accessing connector from geometry, see we do.
Synchronized they are, break not it shall.
81-81: Edge container in new namespace, neat.
Implementation wise, no flaw found.
333-334: In-edge sorting logic, see I do.
Complex it is, but stable it appears.
340-341: Out-edge sorting logic, consistent with in-edge approach.
No mismatch present, good.
356-357: Edge pointer usage, be mindful of boundaries we shall.
Implementation looks safe, yes.
369-369: Next segment pointer retrieval, correct.
Confusion in indexing I sense not.
416-416: Prepare vector for edges, wise approach.
Space allocated, memory usage fine.
419-420: Edge pointer assignment, careful you are.
Null checks handled, presumably they are.
439-439: Edge pointer again retrieved.
Uniform usage, maintain you must.
475-475: Seeds vector for final collection, clarity in code I see.
Limit memory usage, keep an eye you should.
480-481: Filtering edges by level, stable logic.
Time complexity watchers, be.
492-493: Sorting seeds with custom comparator, good arrangement.
No sign of error, proceed.
524-525: Reverse iteration over edges, well-structured.
Mark them as collected, yes.
643-644: createSeeds method uses Experimental namespace, consistent it is.
No mismatch with runGbts_TrackFinder arguments, I sense.
709-710: Overloaded createSeeds returning vector, convenience it brings.
Implementation correct.
716-716: End of namespace, align with Experimental you do.
Proper scoping, no concern I have.Core/include/Acts/TrackFinding/GbtsConnector.hpp (5)
18-18: Namespace shift to Experimental, key to your plan.
Steady, the refactor remains.
32-32: LayerGroup constructor uses experimental connections, yes.
Uniform changes made, good.
36-36: m_sources vector, well updated to the new namespace.
No functional difference introduced, see I do.
45-47: m_connMap storing unique_ptr to Experimental connections.
Memory ownership, properly handled it is.
50-50: Closing namespace scope, nicely placed.
Conflicts with build, foresee I do not.Core/src/TrackFinding/RoiDescriptor.cpp (1)
Line range hint
8-8: Address the TODO comment before the namespace migration, you must.Update to C++17 style, this code requires. Before moving to experimental namespace, modernize the codebase we should.
Help with the C++17 modernization, shall I? Generate the updated code, I can.
Core/include/Acts/Seeding/GbtsBase.hpp (1)
26-43: Consistent namespace changes, I sense. Yet verification we need.To experimental namespace, all GbtsSP types have moved. Breaking change this is, but necessary for future flexibility.
✅ Verification successful
Consistent, the namespace changes are. Complete, the migration is.
Through the Force, searched the entire codebase I have. All GbtsSP occurrences, in the Experimental namespace they reside. No remnants of the old namespace pattern, detected were. Approve this change, I do.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining non-experimental GbtsSP usage rg "Acts::GbtsSP<" --type cppLength of output: 31
Script:
#!/bin/bash # Verify both namespace patterns and ensure files are detected echo "=== Checking file detection ===" fd -e hpp -e cpp echo -e "\n=== Checking old namespace pattern ===" rg "Acts::GbtsSP<" --type cpp -l echo -e "\n=== Checking new namespace pattern ===" rg "Acts::Experimental::GbtsSP<" --type cpp -l echo -e "\n=== Using AST-based search for more accuracy ===" ast-grep --pattern 'Acts::GbtsSP<$_>' ast-grep --pattern 'Acts::Experimental::GbtsSP<$_>'Length of output: 82435
Script:
#!/bin/bash # Search specifically in Seeding directory with various patterns echo "=== Searching in Seeding directory ===" fd -e hpp -e cpp . | grep -i "seeding" echo -e "\n=== Checking for GbtsSP with various patterns ===" rg -i "GbtsSP" --type cpp echo -e "\n=== Checking for specific file ===" fd "GbtsBase.hpp"Length of output: 8110
Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/GbtsSeedingAlgorithm.hpp (1)
38-39: Mixed namespaces in Config, confusion they bring. Hmmmm.Acts::Experimental::SeedFinderGbtsConfig<SimSpacePoint> seedFinderConfig; Acts::SeedFinderOptions seedFinderOptions;Consistency in namespace usage, we seek. Consider moving SeedFinderOptions to experimental namespace or document the reasoning for mixed usage.
Core/include/Acts/Seeding/SeedFinderGbtsConfig.hpp (1)
Line range hint
11-11: Another TODO about C++17 modernization, I find.Before the experimental namespace journey continues, modernize this code we must.
Help with the C++17 modernization, shall I? Generate the updated code, I can.
Core/include/Acts/Seeding/SeedFinderGbts.hpp (1)
30-30: Hmm, moved to experimental namespace, this code has.Consistent with the PR objectives, the transition to
Acts::Experimentalnamespace is. A breaking change this represents, affecting downstream dependencies it will. Properly documented in the PR description, this change is.Run this verification script, we must, to ensure all dependencies are updated:
Also applies to: 68-71, 74-76, 86-87
✅ Verification successful
Verified, the namespace change is. Safe to proceed, we may.
Properly isolated to seeding functionality, this change is. Other usages of Acts namespace, in unrelated components they remain. No breaking changes in dependent code, detected they were not.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old namespace rg "namespace\s+Acts\s*{" --type cpp rg "Acts::[^E][^x][^p]" --type cpp # Find direct Acts:: usage excluding ExperimentalLength of output: 82728
Core/include/Acts/Seeding/GbtsTrackingFilter.hpp (1)
36-36: Comprehensive namespace migration, performed with precision it was.Updated all references to
Acts::Experimentalnamespace, this file has. Method signatures, member variables, and template parameters, properly aligned they are. Complex tracking algorithms, unchanged in functionality they remain.Verify the completeness of namespace migration, we shall:
Also applies to: 102-102, 115-119, 122-122, 156-156, 176-176, 182-182, 213-214, 221-221, 382-382, 384-384
✅ Verification successful
Proper namespace migration, confirmed it is!
Searched through the Force, I have. All references to
GbtsEdgeand related types, properly moved toActs::Experimentalnamespace, they are. No remnants of old namespace usage, found I have. Consistent throughout the codebase, these changes are.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any missed type references rg "Acts::(GbtsEdge|TrigInDetSiLayer)" --type cppLength of output: 51
Script:
#!/bin/bash # Search for any GbtsEdge usage patterns rg "GbtsEdge" --type cpp -A 2 -B 2 # Also check for specific file content fd "GbtsTrackingFilter" -t f -x cat {} # Look for any remaining Acts:: namespace usage in relevant files fd "Gbts" -t f -x rg "Acts::" {} -lLength of output: 22882
Examples/Python/src/TrackFinding.cpp (1)
199-199: Approve the namespace change for GBTS components, I do.Aligned with the PR objectives, this change is. To experimental namespace, moving GBTS components we are. Hmmmm.
Run this script to verify the consistency of GBTS namespace changes across the codebase, we must:
✅ Verification successful
Consistent, the namespace changes are. Moved to experimental namespace, all GBTS components have been.
In the Force, I sense no disturbance. Complete and thorough, the migration is. Hmmmm.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all GBTS components are consistently moved to experimental namespace echo "Search for any remaining GBTS components in Acts namespace..." rg "Acts::(?!Experimental::).*Gbts" --type cpp echo "Verify GBTS components in experimental namespace..." rg "Acts::Experimental::.*Gbts" --type cppLength of output: 6455
Script:
#!/bin/bash # Search for any GBTS components, then exclude experimental namespace matches rg "Acts::[^E][^x][^p].*Gbts|Acts::$|Acts::G" --type cpp | grep -v "Acts::Experimental"Length of output: 65654
|
@andiwand thank you so much for looking through this. I made all the suggested changes in my last 3 commits. I've removed all cases of Acts::Experimental:: in core, but just to check these are still needed in examples as that is a different namespace? |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
Core/include/Acts/Seeding/GbtsTrackingFilter.hpp (1)
Line range hint
112-112: Magic number as macro, concerning this is!Hard-coded value
MAX_EDGE_STATEas macro, a code smell it is. To a constexpr variable, convert it we should.-#define MAX_EDGE_STATE 2500 +static constexpr std::size_t MAX_EDGE_STATE = 2500;Core/include/Acts/TrackFinding/GbtsConnector.hpp (1)
Line range hint
31-44: Raw pointers, dangerous they are. Encapsulation, missing it is.Two concerns I sense:
- Raw pointers in
LayerGroup::m_sourcesvector, ownership unclear they are. Considerstd::shared_ptrorstd::reference_wrapper.- Public members in
GbtsConnector, exposed they should not be.The force guides us to this solution:
class GbtsConnector { +private: + float m_etaBin{}; + std::map<int, std::vector<struct LayerGroup>> m_layerGroups; + std::map<int, std::vector<std::unique_ptr<GbtsConnection>>> m_connMap; + public: struct LayerGroup { - LayerGroup(unsigned int l1Key, const std::vector<const GbtsConnection *> &v) - : m_dst(l1Key), m_sources(v) {} + LayerGroup(unsigned int l1Key, + const std::vector<std::reference_wrapper<const GbtsConnection>> &v) + : m_dst(l1Key), m_sources(v) {} unsigned int m_dst; - std::vector<const GbtsConnection *> m_sources; + std::vector<std::reference_wrapper<const GbtsConnection>> m_sources; }; GbtsConnector(std::ifstream &inFile); - float m_etaBin{}; - std::map<int, std::vector<struct LayerGroup>> m_layerGroups; - std::map<int, std::vector<std::unique_ptr<GbtsConnection>>> m_connMap; + + float getEtaBin() const { return m_etaBin; } + const auto& getLayerGroups() const { return m_layerGroups; } + const auto& getConnMap() const { return m_connMap; } };
🧹 Nitpick comments (9)
Core/include/Acts/Seeding/GbtsTrackingFilter.hpp (5)
Line range hint
1-21: Update the TODO comment to modern C++17 style, we must!Outdated TODO comment at the top, spotted I have. Modern C++17 practices, embrace we should.
-// TODO: update to C++17 style +// TODO: Modernize includes to C++17 style using <filesystem>, <string_view>, etc.
Line range hint
222-379: Complex update method, refactoring needs it does!Long and complex, the update method is. Many magic numbers and calculations it contains. To smaller, focused methods, split this we should. More readable and maintainable, the code will become.
Consider extracting these logical segments into separate methods:
- Covariance validation
- Measurement system transformation
- State prediction
- Chi-square calculation
- Kalman filter update
Line range hint
244-254: Validation messages, improve we must!Warning messages for negative covariance, better context they need. The component names and expected ranges, include we should.
- ACTS_WARNING("Negative covariance detected in X components: " - << "cov[2][2]=" << ts.m_Cx[2][2] << " cov[1][1]=" - << ts.m_Cx[1][1] << " cov[0][0]=" << ts.m_Cx[0][0]); + ACTS_WARNING("Invalid negative covariance in X components (expected >= 0): " + << "acceleration=" << ts.m_Cx[2][2] + << ", velocity=" << ts.m_Cx[1][1] + << ", position=" << ts.m_Cx[0][0]);
Line range hint
380-392: Member variable organization, improve we can!Public logger accessor but private unique_ptr, confusing this is. Group related members together and document their purpose, we should.
private: + // Geometry and storage const std::vector<TrigInDetSiLayer>& m_geo; std::vector<GbtsEdge<external_spacepoint_t>>& m_segStore; + // State management std::vector<GbtsEdgeState<external_spacepoint_t>*> m_stateVec; GbtsEdgeState<external_spacepoint_t> m_stateStore[MAX_EDGE_STATE]; int m_globalStateCounter{0}; + // Logging const Acts::Logger& logger() const { return *m_logger; } std::unique_ptr<const Acts::Logger> m_logger{nullptr};
Line range hint
178-192: Container choice, reconsider we should!Using std::list for temporary storage, unnecessary overhead it creates. When size known not is, vector more efficient would be.
- std::list<GbtsEdge<external_spacepoint_t>*> lCont; + std::vector<GbtsEdge<external_spacepoint_t>*> lCont;Core/include/Acts/TrackFinding/GbtsConnector.hpp (2)
Line range hint
12-13: Address the TODO comments, young Padawan must.Track these improvements we should:
- Modernize to C++17 style
- Consider moving to detail subdirectory
Help you with these tasks, I can. Create an issue to track these improvements, shall I?
Line range hint
20-27: Encapsulation improvements suggest, I do.Public members, a path to the dark side they are. Consider these improvements:
- Private members with accessor methods
- Constructor parameter validation
Example of the way, this is:
struct GbtsConnection { - public: +private: unsigned int m_src, m_dst; std::vector<int> m_binTable; + +public: GbtsConnection(unsigned int s, unsigned int d); + + unsigned int getSource() const { return m_src; } + unsigned int getDestination() const { return m_dst; } + const std::vector<int>& getBinTable() const { return m_binTable; } };Core/include/Acts/Seeding/SeedFinderGbts.ipp (2)
79-79: Performance optimization for edge storage, suggest I do!For better memory management and performance, consider these improvements:
- Pre-allocate vector capacity based on expected size
- Use reserve() for dynamic vectors
- Consider using std::vector instead of pointers for better cache locality
- std::vector<GbtsEdge<external_spacepoint_t>*> v_old; + std::vector<GbtsEdge<external_spacepoint_t>> v_old; + v_old.reserve(nEdges);Also applies to: 81-81, 333-333, 339-339, 354-354, 366-366, 412-412, 415-415, 434-434, 469-469, 474-474, 485-486
Line range hint
518-524: Simplify iterator usage with range-based for loop, we should!More readable and less error-prone, range-based for loops are.
- for (typename std::vector< - GbtsEdge<external_spacepoint_t>*>::reverse_iterator sIt = - rs.m_vs.rbegin(); - sIt != rs.m_vs.rend(); ++sIt) { - (*sIt)->m_level = -1; // mark as collected + for (auto* edge : std::ranges::reverse_view(rs.m_vs)) { + edge->m_level = -1; // mark as collected
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Core/include/Acts/Seeding/GbtsBase.hpp(1 hunks)Core/include/Acts/Seeding/GbtsGeometry.hpp(4 hunks)Core/include/Acts/Seeding/GbtsTrackingFilter.hpp(9 hunks)Core/include/Acts/Seeding/SeedFinderGbts.hpp(4 hunks)Core/include/Acts/Seeding/SeedFinderGbts.ipp(14 hunks)Core/include/Acts/TrackFinding/GbtsConnector.hpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Core/include/Acts/Seeding/SeedFinderGbts.hpp
- Core/include/Acts/Seeding/GbtsBase.hpp
- Core/include/Acts/Seeding/GbtsGeometry.hpp
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: unused_files
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: build_debug
- GitHub Check: linux_ubuntu
🔇 Additional comments (6)
Core/include/Acts/Seeding/GbtsTrackingFilter.hpp (1)
22-23: Approve the namespace change, I do!To the experimental namespace, moved this code has been. Wise decision this is, for future breaking changes to avoid.
Also applies to: 396-397
Core/include/Acts/TrackFinding/GbtsConnector.hpp (1)
18-18: Wise move to experimental namespace, this is.Aligns with PR objectives, this change does. Breaking changes in future, it prevents.
Core/include/Acts/Seeding/SeedFinderGbts.ipp (4)
31-31: Approve namespace migration, I do!To experimental namespace moved, this code has been. Wise decision for development flexibility, this is.
62-63: Consistent parameter types across method signatures, ensure we must!Removed redundant
Acts::prefix from parameter types, maintaining consistency across all method signatures. Good practice, this is!Also applies to: 637-638, 703-704
710-710: Complete the namespace migration is, approve I do!Clean and consistent namespace usage throughout the file. Ready for experimental features, this code now is!
Line range hint
4-4: Update to C++17 style, the TODO comment requests!Modernize the codebase, we should. Consider these C++17 features:
- structured bindings
- if constexpr
- std::optional
Run this script to identify modernization opportunities:
There was a problem hiding this comment.
two more minor suggestions
@andiwand thank you so much for looking through this. I made all the suggested changes in my last 3 commits. I've removed all cases of Acts::Experimental:: in core, but just to check these are still needed in examples as that is a different namespace?
yes the namespace looks good 👍 some definitions just can be shortened after setting the namespace
Co-authored-by: Andreas Stefl <stefl.andreas@gmail.com>
|
Moving all GBTS files to Acts::Experimental namespace
--- END COMMIT MESSAGE ---
This will break the athena build but I have a MR ready for athena MR for Canary
Moving to the experimental namespace to help with further development so they will not be breaking changes.
I was hoping to get this merged in time for the v39.0.0 release, if possible
Summary by CodeRabbit
Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
Namespace Changes
ActstoActs::Experimentalnamespace.Experimental Features
GbtsGeometry,GbtsEdge,SeedFinderGbts, andRoiDescriptor.Code Structure
Affected Components
These changes suggest ongoing development and refinement of tracking-related components in the Acts library.