chore: Some magnetic field code cleanup #3982
Conversation
WalkthroughChanges made to various header and source files focus on removing unnecessary include directives and simplifying dependencies across the codebase. Notable modifications include the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
![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 (4)
Core/include/Acts/Material/SurfaceMaterialMapper.hpp (2)
Line range hint
39-65: Documentation enhancement, suggest I do.Clear the process steps are, but examples of usage, help young padawans they would. A code snippet showing material track creation and mapping, valuable would be.
Consider adding example usage like this:
// Example usage GeometryContext gctx; MagneticFieldContext mctx; auto state = mapper.createState(gctx, mctx, trackingGeometry); // Create and map a material track RecordedMaterialTrack track; // ... populate track ... mapper.mapMaterialTrack(state, track); // Finalize the mapping mapper.finalizeMaps(state);
Line range hint
66-108: Move semantics, optimize State struct we should.Strong with RAII, this class is. Yet for State struct, move operations explicitly declare we could. Performance benefits, bring this would.
struct State { State(const GeometryContext& gctx, const MagneticFieldContext& mctx) : geoContext(gctx), magFieldContext(mctx) {} + + // Enable move operations + State(State&&) noexcept = default; + State& operator=(State&&) noexcept = default; + + // Disable copy operations + State(const State&) = delete; + State& operator=(const State&) = delete;Examples/Io/Root/src/RootBFieldWriter.cpp (2)
Line range hint
176-186: Consider extracting field value conversion, you should.Repeated unit conversions, I see. A helper function, beneficial it would be.
+ // Helper function to convert field values to desired units + Acts::Vector3 convertFieldToUnits(const Acts::Vector3& rawField) { + return rawField / Acts::UnitConstants::T; + } Acts::Vector3 bField = config.bField->getFieldUnchecked(position); - Bx = bField.x() / Acts::UnitConstants::T; - By = bField.y() / Acts::UnitConstants::T; - Bz = bField.z() / Acts::UnitConstants::T; + auto convertedField = convertFieldToUnits(bField); + Bx = convertedField.x(); + By = convertedField.y(); + Bz = convertedField.z();
Line range hint
1-277: Architecture advice for future improvements, share I will.Large function with multiple responsibilities, this is. Consider splitting into smaller functions for:
- Configuration validation
- Cartesian coordinate handling
- Cylindrical coordinate handling
- File operations
Easier to maintain and test, it would be.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
Core/include/Acts/MagneticField/InterpolatedBFieldMap.hpp(0 hunks)Core/include/Acts/MagneticField/MagneticFieldProvider.hpp(0 hunks)Core/include/Acts/Material/SurfaceMaterialMapper.hpp(1 hunks)Core/include/Acts/Navigation/DetectorNavigator.hpp(0 hunks)Core/include/Acts/Propagator/StraightLineStepper.hpp(0 hunks)Core/src/MagneticField/SolenoidBField.cpp(0 hunks)Core/src/Propagator/SympyStepper.cpp(0 hunks)Core/src/Vertexing/HelicalTrackLinearizer.cpp(0 hunks)Examples/Io/Root/include/ActsExamples/Io/Root/RootBFieldWriter.hpp(0 hunks)Examples/Io/Root/src/RootBFieldWriter.cpp(3 hunks)Examples/Python/src/MagneticField.cpp(0 hunks)Tests/UnitTests/Core/MagneticField/ConstantBFieldTests.cpp(0 hunks)Tests/UnitTests/Core/MagneticField/InterpolatedBFieldMapTests.cpp(0 hunks)Tests/UnitTests/Core/MagneticField/SolenoidBFieldTests.cpp(0 hunks)Tests/UnitTests/Core/Propagator/ExtrapolatorTests.cpp(0 hunks)Tests/UnitTests/Core/Utilities/BFieldMapUtilsTests.cpp(0 hunks)
💤 Files with no reviewable changes (14)
- Core/include/Acts/MagneticField/MagneticFieldProvider.hpp
- Tests/UnitTests/Core/MagneticField/SolenoidBFieldTests.cpp
- Tests/UnitTests/Core/MagneticField/ConstantBFieldTests.cpp
- Tests/UnitTests/Core/Utilities/BFieldMapUtilsTests.cpp
- Core/src/Vertexing/HelicalTrackLinearizer.cpp
- Core/include/Acts/Propagator/StraightLineStepper.hpp
- Core/include/Acts/MagneticField/InterpolatedBFieldMap.hpp
- Core/src/MagneticField/SolenoidBField.cpp
- Tests/UnitTests/Core/Propagator/ExtrapolatorTests.cpp
- Core/include/Acts/Navigation/DetectorNavigator.hpp
- Examples/Io/Root/include/ActsExamples/Io/Root/RootBFieldWriter.hpp
- Core/src/Propagator/SympyStepper.cpp
- Tests/UnitTests/Core/MagneticField/InterpolatedBFieldMapTests.cpp
- Examples/Python/src/MagneticField.cpp
🔇 Additional comments (4)
Core/include/Acts/Material/SurfaceMaterialMapper.hpp (1)
Line range hint 1-225: Hmmmm... Verify the removal of includes, we must.
Removed, three includes have been. Yet ensure we must, that no remnants of their usage remain. Through the Force, sense no immediate issues I do, but verify we shall.
Examples/Io/Root/src/RootBFieldWriter.cpp (3)
178-178: Approve namespace qualification, hmmmm.
Explicit namespace qualification, added you have. Clearer and safer, the code becomes. Prevent naming conflicts, this will.
267-267: Approve namespace qualification for VectorHelpers, I do.
Wise choice it is, to qualify VectorHelpers with Acts namespace. Consistency with codebase standards, this maintains.
265-268: Verify field conversion in cylindrical coordinates, we must.
In cylindrical coordinates, consistent unit conversion for both Br and Bz components, ensure we should.
✅ Verification successful
Let me examine the field conversion implementations in more detail, I must. Consistent patterns across the codebase, we shall find.
Consistent and correct, the field unit conversions are, young padawan.
Through the Force, I sense that both Br and Bz components are properly converted using Acts::UnitConstants::T. The codebase reveals a harmonious pattern where magnetic field components in cylindrical coordinates consistently use the same unit conversion:
- In RootBFieldWriter.cpp:
Bz = bField.z() / Acts::UnitConstants::T;
Br = Acts::VectorHelpers::perp(bField) / Acts::UnitConstants::T;Throughout the codebase, this pattern of conversion is maintained with great discipline. The Force is strong with these unit conversions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of field unit conversion in cylindrical coordinates
rg "UnitConstants::T" --type cpp
Length of output: 2732
Script:
#!/bin/bash
# Search for field conversion implementations in cylindrical coordinates
ast-grep --pattern 'perp($field)'
Length of output: 35
Script:
#!/bin/bash
# Search for VectorHelpers usage
rg "VectorHelpers" -A 3 --type cpp
Length of output: 66041
Script:
#!/bin/bash
# Look for field unit conversion functions or utilities
rg "convertField|BFieldUnit" --type cpp -A 2
Length of output: 10624
|
acts-project-service
added
the
Breaks Athena build
using namespacepulled out of #3951
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
getFieldfunction to provide clearer feedback on failures.Documentation
Chores