refactor: modernise map utils #3615
Conversation
WalkthroughEnhance const-correctness and efficiency, the changes do. In Changes
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
Core/include/Acts/Utilities/Helpers.hpp (1)
226-244: Documentation clarity, strong it is. Yet stronger it could be!Well documented the function is, but warn about parameter modification, we must. Add to documentation, we should:
/// Computes the minimum, maximum, and bin count for a given vector of values. /// +/// @warning This function modifies the input vector by sorting it. +/// /// This function processes a vector of doubles to compute:Core/src/MagneticField/BFieldMapUtils.cpp (2)
85-101: Duplicate code intransformBField, I sense.The
transformBFieldlambda appears in multiple places. Extract into a common helper function, consider you should, to improve maintainability.Create a helper function like this:
Vector3 transformBField(const Vector2& field, const Vector3& pos) { const double rSinTheta2 = pos.x() * pos.x() + pos.y() * pos.y(); double cosPhi = 1.; double sinPhi = 0.; if (rSinTheta2 > std::numeric_limits<double>::epsilon()) { const double invRsinTheta = 1. / sqrt(rSinTheta2); cosPhi = pos.x() * invRsinTheta; sinPhi = pos.y() * invRsinTheta; } return Vector3(field.x() * cosPhi, field.x() * sinPhi, field.y()); }Then, reuse this function in your lambdas.
Also applies to: 219-230
42-46: Repeated code in min/max calculations, see I do.Calculations of min, max, and bin counts duplicated they are. Refactor to reduce duplication, consider you should. Perhaps a helper function or a template, use you can.
Also applies to: 121-127
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
Core/include/Acts/MagneticField/BFieldMapUtils.hpp(2 hunks)Core/include/Acts/Utilities/Helpers.hpp(1 hunks)Core/src/MagneticField/BFieldMapUtils.cpp(4 hunks)Core/src/Material/MaterialMapUtils.cpp(7 hunks)
🔇 Additional comments (10)
Core/src/Material/MaterialMapUtils.cpp (5)
15-15: Wise addition of Helpers.hpp, I sense.
Properly placed among utility includes, this header is. Support the new getMinMaxAndBinCount function, it does.
47-48: Elegant simplification achieved, it has been.
Replaced complex calculations with the new utility function, we have. Cleaner and more maintainable, the code becomes.
60-60: Wise optimization, moving nIndices outside the loops is.
Performance benefits, this change brings. Avoid repeated array construction in loops, we now do.
104-106: Consistent with RZ mapper changes, these modifications are.
Mirror the improvements from RZ mapper, these changes do:
- Use getMinMaxAndBinCount for all dimensions
- Move nIndices outside loops
- Maintain consistent style
Balance and harmony in the code, I sense.
Also applies to: 118-118
47-48: Verify the test coverage of getMinMaxAndBinCount, we must.
Critical this utility function has become, as handle important calculations, it does.
Also applies to: 104-106
✅ Verification successful
Sufficient test coverage for getMinMaxAndBinCount, I sense.
Through the Force, observe I do that MaterialMapUtilsTests.cpp thoroughly tests the results of getMinMaxAndBinCount through the material mapper creation. Validated are the bin counts, minima, and maxima through explicit checks:
- Test case 'materialmap_creation' verifies mapper dimensions
- Comprehensive checks for both RZ and XYZ configurations exist
- Boundary conditions and transformations, tested they are
The way of the Jedi, thorough testing is. Peace of mind, we have.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests of getMinMaxAndBinCount
rg -l "getMinMaxAndBinCount.*Test" Tests/
Length of output: 43
Script:
#!/bin/bash
# Search for test files related to MaterialMapUtils
fd "MaterialMap.*Test" Tests/
# Search for any usage of getMinMaxAndBinCount in test files
rg "getMinMaxAndBinCount" Tests/ -A 5
# Look for the actual implementation to understand the context better
ast-grep --pattern 'getMinMaxAndBinCount($_) {
$$$
}'
Length of output: 485
Script:
#!/bin/bash
# Check content of MaterialMapUtilsTests.cpp specifically
cat Tests/UnitTests/Core/Utilities/MaterialMapUtilsTests.cpp
# Look for the implementation in MaterialMapUtils
cat Core/src/Material/MaterialMapUtils.cpp
Length of output: 10659
Core/include/Acts/MagneticField/BFieldMapUtils.hpp (4)
78-78: Wise changes to const reference, I see!
Prevent unnecessary copies and maintain const-correctness, this change does. A path to better performance, it is.
140-140: Hmmmm... const reference for bField parameter, good it is!
Similar to fieldMapRZ, this change brings balance to the Force... err... const-correctness.
148-150: Const references for parameters, added you have. Strong with the Force, this change is!
Improved efficiency through const references for rLim, zLim, and nBins parameters, I observe. Memory copies, reduced they are.
However, verify the impact on callers, we must.
Also applies to: 157-159
✅ Verification successful
The Force guides me to examine the implementation and usage patterns more deeply, to ensure const correctness throughout the codebase, hmm.
Safe, the const references are. Verified, their usage has been.
Through the Force, seen I have that all callers pass temporary pairs or direct values that bind properly to const references. In the test files and benchmark code, observe we do:
solenoidFieldMap({rMin, rMax}, {zMin, zMax}, {nBinsR, nBinsZ}, field);And in implementation, properly const references are used:
Acts::solenoidFieldMap(const std::pair<double, double>& rLim,
const std::pair<double, double>& zLim,
const std::pair<std::size_t, std::size_t>& nBins,
const SolenoidBField& field)No disturbance in the Force do I sense. Safe and efficient, this change is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to solenoidFieldMap to ensure they work with const references
rg "solenoidFieldMap\s*\(" --type cpp
Length of output: 499
Script:
#!/bin/bash
# Check the actual usage in the test files to ensure const correctness
cat Tests/IntegrationTests/InterpolatedSolenoidBFieldTest.cpp Tests/Benchmarks/SolenoidFieldBenchmark.cpp
Length of output: 12607
Script:
#!/bin/bash
# Check the implementation to verify const correctness
cat Core/src/MagneticField/BFieldMapUtils.cpp
Length of output: 9338
Line range hint 1-162: Documentation, extensive it is. But examples, outdated they might be.
The function documentation shows examples using lambda functions. Verify their correctness with the new const parameters, we should.
✅ Verification successful
Correct, the examples in documentation are, hmmmm. Match the actual usage they do.
In the test files, observe I do, that the lambda functions perfectly align with the documentation examples:
For RZ mapping:
auto localToGlobalBin_rz = [](std::array<std::size_t, 2> binsRZ,
std::array<std::size_t, 2> nBinsRZ) {
return (binsRZ.at(1) * nBinsRZ.at(0) + binsRZ.at(0));
};For XYZ mapping:
auto localToGlobalBin_xyz = [](std::array<std::size_t, 3> binsXYZ,
std::array<std::size_t, 3> nBinsXYZ) {
return (binsXYZ.at(0) * (nBinsXYZ.at(1) * nBinsXYZ.at(2)) +
binsXYZ.at(1) * nBinsXYZ.at(2) + binsXYZ.at(2));
};Accurate and up-to-date, the documentation examples are. No changes needed, hmmmm.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for actual usage patterns of these utility functions
rg "fieldMapRZ|fieldMapXYZ|solenoidFieldMap" -B 2 -A 2 --type cpp
Length of output: 10343
Script:
#!/bin/bash
# Search for actual lambda function implementations in test files
rg "localToGlobalBin_rz|localToGlobalBin_xyz" -B 2 -A 5 --type cpp Tests/
Length of output: 14428
Script:
#!/bin/bash
# Look at the actual implementation of these functions
ast-grep --pattern 'auto map_$_ = Acts::fieldMap$_($$$)'
Length of output: 58
Core/include/Acts/Utilities/Helpers.hpp (1)
224-225: Wise choice of namespace, young padawan!
In the detail namespace, internal implementation details belong. Protected from misuse by users, this function now is. Hmmmm.
Also applies to: 265-265
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
Core/src/MagneticField/BFieldMapUtils.cpp (1)
158-163: Consider extracting common arithmetic pattern, you should.Repeated pattern of absolute difference calculation, I see. A helper function, beneficial it would be.
+ auto calcAbsDiff = [](std::size_t val, std::size_t binCount) { + return std::abs(static_cast<std::ptrdiff_t>(val) - + static_cast<std::ptrdiff_t>(binCount)); + }; std::size_t l = calcAbsDiff(i, xBinCount); std::size_t m = calcAbsDiff(j, yBinCount); std::size_t n = calcAbsDiff(k, zBinCount);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Core/src/MagneticField/BFieldMapUtils.cpp(4 hunks)
🧰 Additional context used
📓 Learnings (1)
Core/src/MagneticField/BFieldMapUtils.cpp (1)
Learnt from: AJPfleger
PR: acts-project/acts#3615
File: Core/src/MagneticField/BFieldMapUtils.cpp:95-98
Timestamp: 2024-11-27T10:23:30.665Z
Learning: In `Core/src/MagneticField/BFieldMapUtils.cpp`, when `sqrt(std::numeric_limits<double>::min())` is used, using `std::numeric_limits<double>::min()` in comparisons is acceptable as there is no risk of division by zero.
🔇 Additional comments (6)
Core/src/MagneticField/BFieldMapUtils.cpp (6)
15-15: Wise additions to the includes, these are!
The Force is strong with these changes:
Helpers.hppbrings the newgetMinMaxAndBinCountfunctioncstddefsupports the proper integer type usage, it does
Also applies to: 22-22
43-44: Simplified the code with getMinMaxAndBinCount, we have!
More elegant and maintainable, the solution is. Reduces duplication and potential errors, it does.
71-72: Safe integer arithmetic, you now use!
Wise choice, using std::ptrdiff_t for signed arithmetic is. Prevents undefined behavior from negative values, it does.
96-99: Careful with floating-point comparisons, you are!
From the learnings, I see that std::numeric_limits<double>::min() acceptable it is, as sqrt() of this value you take. Prevents division by zero while maintaining precision, it does.
122-124: Clean and consistent use of getMinMaxAndBinCount, this is!
Harmonious with the changes in fieldMapRZ, these modifications are.
197-199: Clean initialization with structured bindings, you have achieved!
More readable and maintainable, the code becomes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Core/src/MagneticField/BFieldMapUtils.cpp (1)
151-154: Helper function calcAbsDiff, created you have!Encapsulates index calculation logic, it does. Improves readability and maintainability, this does.
Consider moving this helper to a more general utility location if used elsewhere in the codebase:
+ namespace detail { + inline std::size_t calcAbsDiff(std::size_t val, std::size_t binCount) { + return std::abs(static_cast<std::ptrdiff_t>(val) - + static_cast<std::ptrdiff_t>(binCount)); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Core/src/MagneticField/BFieldMapUtils.cpp(4 hunks)
🧰 Additional context used
📓 Learnings (1)
Core/src/MagneticField/BFieldMapUtils.cpp (2)
Learnt from: AJPfleger
PR: acts-project/acts#3615
File: Core/src/MagneticField/BFieldMapUtils.cpp:214-231
Timestamp: 2024-12-02T10:20:44.377Z
Learning: In the C++ file `Core/src/MagneticField/BFieldMapUtils.cpp`, when passing methods to constructors in functions like `fieldMapRZ` and `solenoidFieldMap`, prefer to define transforms in each function separately for clarity, even if the methods are similar.
Learnt from: AJPfleger
PR: acts-project/acts#3615
File: Core/src/MagneticField/BFieldMapUtils.cpp:95-98
Timestamp: 2024-11-27T10:23:30.665Z
Learning: In `Core/src/MagneticField/BFieldMapUtils.cpp`, when `sqrt(std::numeric_limits<double>::min())` is used, using `std::numeric_limits<double>::min()` in comparisons is acceptable as there is no risk of division by zero.
🔇 Additional comments (7)
Core/src/MagneticField/BFieldMapUtils.cpp (7)
15-15: Wise additions to the includes, these are!
Added Helpers.hpp for getMinMaxAndBinCount utility and cstddef for std::ptrdiff_t, improve type safety they do.
Also applies to: 22-22
43-44: Simplified bin calculation with getMinMaxAndBinCount, you have!
More maintainable and efficient, the code becomes. A wise refactoring, this is.
71-72: Improved type safety with std::ptrdiff_t, you have!
Safer arithmetic operations with indices, this ensures. Previous feedback, you have heeded.
85-102: Separate transform definitions, maintain you do!
As learned from previous discussions, clearer it is to define transforms separately when passing to constructor. Wise decision, this is.
122-124: Consistent use of getMinMaxAndBinCount, maintain you do!
Harmonious with fieldMapRZ changes, this implementation is. Consistency in the codebase, it brings.
200-202: Modern C++ features, embrace you do!
Structured bindings for parameter handling, improve readability they do.
228-231: Consistent with previous approval, this implementation is!
Use of std::numeric_limits<double>::min() with sqrt operation, safe it is. Previous discussions confirm this approach.
|
Summary by CodeRabbit
New Features
Refactor
Bug Fixes