feat: Add a multi-region constant B-field provider #3927
Conversation
WalkthroughA new class 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 (
|
stephenswat
changed the title
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: 2
🧹 Outside diff range and nitpick comments (3)
Core/include/Acts/MagneticField/MultiRangeBField.hpp (2)
52-52: Unnecessary semicolon after function definition, there is. Remove it, you should.The semicolon after the function closing brace is redundant.
Apply this diff to improve code clarity:
- }; + }
104-104: Unnecessary semicolon after function definition, there is. Remove it, you should.The semicolon after the function closing brace is redundant.
Apply this diff to improve code clarity:
- }; + }Examples/Python/src/Geometry.cpp (1)
495-509: Approve the changes, I do. A small suggestion, I have.Clean and well-structured, this implementation is. Follows the established patterns for
RangeXDDim1andRangeXDDim2, it does. Proper initialization of ranges using theshrinkmethod, I observe.For consistency with other dimensional implementations and improved readability, consider adding a descriptive comment, you should.
Apply this change, you may:
{ + // Python bindings for 3D range implementation used in magnetic field calculations using RangeXDDim3 = Acts::RangeXD<3u, double>; py::class_<RangeXDDim3>(m, "RangeXDDim3")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
Core/include/Acts/MagneticField/MultiRangeBField.hpp(1 hunks)Examples/Python/src/Geometry.cpp(1 hunks)Examples/Python/src/MagneticField.cpp(2 hunks)Examples/Python/tests/test_magnetic_field.py(1 hunks)Tests/UnitTests/Core/MagneticField/CMakeLists.txt(1 hunks)Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp(1 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp
[error] 20-20: syntax error
(syntaxError)
🔇 Additional comments (4)
Tests/UnitTests/Core/MagneticField/CMakeLists.txt (1)
4-4: LGTM, it does.
The addition of the unit test for MultiRangeBField appropriate is. Correct the test configuration is.
Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp (1)
20-20:
Syntax error detected by static analysis tools, there is. Verify and fix, you must.
Static analysis reports a syntax error on line 20. Ensure that macros and syntax correct are.
Run the following script to identify syntax errors:
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 20-20: syntax error
(syntaxError)
Examples/Python/tests/test_magnetic_field.py (1)
77-112: Good addition of test cases for MultiRangeBField, it is.
Comprehensive the test coverage is, and functionality it validates correctly.
Examples/Python/src/MagneticField.cpp (1)
90-93: Bindings for MultiRangeBField correctly added, they are.
The Python bindings for MultiRangeBField properly defined are, matching the constructor signature.
|
It would be nice to support this in covfie field as well or does it already? |
Of course, that's a good idea. 👍 |
stephenswat
force-pushed
the
feat/multi_range_bfield
branch
from
44edbb8 to
93624ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Examples/Python/tests/test_magnetic_field.py (1)
77-112: Enhance the test coverage with random points, we should.Good tests you have written, but stronger they could become. Like the null_bfield test function above, random point testing add you should.
Apply this enhancement, you could:
def test_multiregion_bfield(): with pytest.raises(TypeError): acts.MultiRangeBField() rs = [ (acts.RangeXDDim3((0, 3), (0, 3), (0, 3)), acts.Vector3(0.0, 0.0, 2.0)), (acts.RangeXDDim3((1, 2), (1, 2), (1, 10)), acts.Vector3(2.0, 0.0, 0.0)), ] f = acts.MultiRangeBField(rs) assert f ctx = acts.MagneticFieldContext() assert ctx fc = f.makeCache(ctx) assert fc + # Random point testing for comprehensive coverage + for _ in range(100): + x = random.uniform(-1.0, 4.0) + y = random.uniform(-1.0, 4.0) + z = random.uniform(-1.0, 11.0) + rv = f.getField(acts.Vector3(x, y, z), fc) + # Assert field values based on point location + # Add appropriate assertions here # Existing specific test cases below...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
Core/include/Acts/MagneticField/MultiRangeBField.hpp(1 hunks)Examples/Python/src/Geometry.cpp(1 hunks)Examples/Python/src/MagneticField.cpp(2 hunks)Examples/Python/tests/test_magnetic_field.py(1 hunks)Tests/UnitTests/Core/MagneticField/CMakeLists.txt(1 hunks)Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Tests/UnitTests/Core/MagneticField/CMakeLists.txt
- Examples/Python/src/MagneticField.cpp
- Core/include/Acts/MagneticField/MultiRangeBField.hpp
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp
[error] 20-20: syntax error
(syntaxError)
🔇 Additional comments (2)
Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp (1)
20-73: Well structured and comprehensive, these tests are!
Cover all essential scenarios, your test cases do:
- Inside first volume, testing is
- Second volume verification, included it is
- Overlap regions, handled they are
- Outside volumes, tested it is also
Pleased with this implementation, I am.
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 20-20: syntax error
(syntaxError)
Examples/Python/src/Geometry.cpp (1)
495-508: Consistent with the Force, this implementation is!
Follow the established patterns of RangeXDDim1 and RangeXDDim2, it does. Clean and well-structured, the code is.
stephenswat
force-pushed
the
feat/multi_range_bfield
branch
from
93624ea to
b55fbc1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
Examples/Python/src/Geometry.cpp (1)
495-509: Approve this implementation, I do.Proper and consistent with its siblings
RangeXDDim1andRangeXDDim2, this implementation is. Well-structured binding for three-dimensional ranges, you have created. Follows the established patterns of pybind11, it does.A suggestion for documentation, I have:
{ + // Binding for three-dimensional range support using RangeXDDim3 = Acts::RangeXD<3u, double>; py::class_<RangeXDDim3>(m, "RangeXDDim3")Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp (2)
20-39: Define tolerance as named constant, hmm yes.Magic number 0.05 for comparison tolerance, I see. Better readability and maintainability, a named constant would provide.
+ constexpr double kFieldTolerance = 0.05; BOOST_AUTO_TEST_CASE(TestMultiRangeBField) { // ... existing code ... - BOOST_CHECK_CLOSE((*r)[0], 0., 0.05); - BOOST_CHECK_CLOSE((*r)[1], 0., 0.05); - BOOST_CHECK_CLOSE((*r)[2], 2., 0.05); + BOOST_CHECK_CLOSE((*r)[0], 0., kFieldTolerance); + BOOST_CHECK_CLOSE((*r)[1], 0., kFieldTolerance); + BOOST_CHECK_CLOSE((*r)[2], 2., kFieldTolerance);🧰 Tools
🪛 cppcheck (2.10-2)
[error] 20-20: syntax error
(syntaxError)
1-74: Additional test cases, we need.Well-structured the tests are, but edge cases we must also verify. Test points exactly on region boundaries and corners, we should.
Add these test cases:
// Test point exactly on boundary { Result<Vector3> r = bfield.getField({3.0, 3.0, 3.0}, bcache); BOOST_CHECK(r.ok()); BOOST_CHECK_CLOSE((*r)[0], 0., kFieldTolerance); BOOST_CHECK_CLOSE((*r)[1], 0., kFieldTolerance); BOOST_CHECK_CLOSE((*r)[2], 2., kFieldTolerance); } // Test point on corner where regions meet { Result<Vector3> r = bfield.getField({1.0, 1.0, 1.0}, bcache); BOOST_CHECK(r.ok()); BOOST_CHECK_CLOSE((*r)[0], 2., kFieldTolerance); BOOST_CHECK_CLOSE((*r)[1], 0., kFieldTolerance); BOOST_CHECK_CLOSE((*r)[2], 0., kFieldTolerance); }🧰 Tools
🪛 cppcheck (2.10-2)
[error] 20-20: syntax error
(syntaxError)
docs/core/magnetic_field.md (1)
240-250: Grammar and formatting improvements, need we do.Consistent with style guide, these changes must be:
-The multi-range constant field allows to model cases where a magnetic field can +The multi-range constant field allows modeling cases where a magnetic field can be described as multiple (potentially overlapping) regions, each of which has its own constant magnetic field. Magnetic field lookup then proceeds by finding the _last_ region in the -user-provided list which contains the requested coordinate and returning the +user-provided list that contains the requested coordinate and returning the corresponding field vector.🧰 Tools
🪛 LanguageTool
[grammar] ~240-~240: Did you mean “modeling”, “modelling”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ... The multi-range constant field allows to model cases where a magnetic field can be des...(ALLOW_TO)
🪛 Markdownlint (0.35.0)
248-248: Expected: asterisk; Actual: underscore
Emphasis style(MD049, emphasis-style)
248-248: Expected: asterisk; Actual: underscore
Emphasis style(MD049, emphasis-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
Core/include/Acts/MagneticField/MultiRangeBField.hpp(1 hunks)Examples/Python/src/Geometry.cpp(1 hunks)Examples/Python/src/MagneticField.cpp(2 hunks)Examples/Python/tests/test_magnetic_field.py(1 hunks)Tests/UnitTests/Core/MagneticField/CMakeLists.txt(1 hunks)Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp(1 hunks)docs/core/magnetic_field.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Tests/UnitTests/Core/MagneticField/CMakeLists.txt
- Examples/Python/tests/test_magnetic_field.py
- Examples/Python/src/MagneticField.cpp
- Core/include/Acts/MagneticField/MultiRangeBField.hpp
🧰 Additional context used
🪛 LanguageTool
docs/core/magnetic_field.md
[grammar] ~240-~240: Did you mean “modeling”, “modelling”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ... The multi-range constant field allows to model cases where a magnetic field can be des...
(ALLOW_TO)
🪛 Markdownlint (0.35.0)
docs/core/magnetic_field.md
248-248: Expected: asterisk; Actual: underscore
Emphasis style
(MD049, emphasis-style)
248-248: Expected: asterisk; Actual: underscore
Emphasis style
(MD049, emphasis-style)
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp
[error] 20-20: syntax error
(syntaxError)
stephenswat
force-pushed
the
feat/multi_range_bfield
branch
from
b55fbc1 to
c6e2dee
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
docs/core/magnetic_field.md (1)
248-248: Emphasis style, inconsistent it is.Underscores for emphasis used, asterisks preferred they are. Consistency maintain, you should.
Apply this diff to correct the emphasis:
- Magnetic field lookup then proceeds by finding the _last_ region in the + Magnetic field lookup then proceeds by finding the *last* region in the🧰 Tools
🪛 Markdownlint (0.35.0)
248-248: Expected: asterisk; Actual: underscore
Emphasis style(MD049, emphasis-style)
248-248: Expected: asterisk; Actual: underscore
Emphasis style(MD049, emphasis-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
Core/include/Acts/MagneticField/MultiRangeBField.hpp(1 hunks)Core/src/MagneticField/CMakeLists.txt(1 hunks)Core/src/MagneticField/MultiRangeBField.cpp(1 hunks)Examples/Python/src/Geometry.cpp(1 hunks)Examples/Python/src/MagneticField.cpp(2 hunks)Examples/Python/tests/test_magnetic_field.py(1 hunks)Tests/UnitTests/Core/MagneticField/CMakeLists.txt(1 hunks)Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp(1 hunks)docs/core/magnetic_field.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Tests/UnitTests/Core/MagneticField/CMakeLists.txt
- Examples/Python/tests/test_magnetic_field.py
- Examples/Python/src/MagneticField.cpp
- Core/include/Acts/MagneticField/MultiRangeBField.hpp
🧰 Additional context used
🪛 Markdownlint (0.35.0)
docs/core/magnetic_field.md
248-248: Expected: asterisk; Actual: underscore
Emphasis style
(MD049, emphasis-style)
248-248: Expected: asterisk; Actual: underscore
Emphasis style
(MD049, emphasis-style)
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp
[error] 20-20: syntax error
(syntaxError)
🔇 Additional comments (5)
Core/src/MagneticField/MultiRangeBField.cpp (2)
15-21: Constructors well-defined, they are.
Initialization with both lvalue and rvalue references handled efficiently.
27-71: Efficient caching mechanism, implemented you have.
Caching of the last matched range optimizes field retrieval performance.
Core/src/MagneticField/CMakeLists.txt (1)
7-7: New source file added correctly, it is.
MultiRangeBField.cpp included in target sources as required.
docs/core/magnetic_field.md (1)
238-256: Additional documentation, beneficial it would be.
Details on performance characteristics, cache implementation benefits, and thread safety guarantees, provide you should.
Consider adding these sections to enrich the documentation:
+### Performance and Implementation Details
+
+The implementation uses a simple caching mechanism to store the last matched
+region, providing improved performance for consecutive lookups within the same
+region. Compared to `ConstantBField`, this implementation has minimal overhead
+when using a single region, and scales linearly with the number of regions.
+
+### Thread Safety
+
+The `MultiRangeBField` is thread-safe when each thread uses its own cache
+instance obtained through `makeCache()`. The field configuration itself is
+immutable after construction.🧰 Tools
🪛 Markdownlint (0.35.0)
248-248: Expected: asterisk; Actual: underscore
Emphasis style
(MD049, emphasis-style)
248-248: Expected: asterisk; Actual: underscore
Emphasis style
(MD049, emphasis-style)
Examples/Python/src/Geometry.cpp (1)
495-508: Exposing 3D range to Python, good this is.
The RangeXDDim3 class added, allowing three-dimensional range handling in Python bindings.
stephenswat
force-pushed
the
feat/multi_range_bfield
branch
from
c6e2dee to
b4c1434
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp (2)
21-31: More descriptive names for test data, suggest I do.Clearer intent, better variable names would show. Consider these changes:
- std::vector<std::pair<RangeXD<3, double>, Vector3>> inputs; + std::vector<std::pair<RangeXD<3, double>, Vector3>> fieldRegions; - inputs.emplace_back( + fieldRegions.emplace_back(
32-72: Additional test scenarios, consider we should.Well-structured your tests are, but stronger they could become. Additional scenarios to consider:
- Test with empty region list
- Test with single region
- Test region priority with complete overlap
Help with implementing these test cases, would you like?
Examples/Python/tests/test_magnetic_field.py (1)
77-112: Use pytest fixtures for test data, recommend I do.Good your tests are, but better organization achieve we can. Create a fixture for field regions and context:
@pytest.fixture def field_regions(): return [ (acts.RangeXDDim3((0, 3), (0, 3), (0, 3)), acts.Vector3(0.0, 0.0, 2.0)), (acts.RangeXDDim3((1, 2), (1, 2), (1, 10)), acts.Vector3(2.0, 0.0, 0.0)), ] @pytest.fixture def field_context(): return acts.MagneticFieldContext()Cleaner and more reusable, your tests will become.
Examples/Python/src/Geometry.cpp (1)
495-508: Approve the implementation, I do. A suggestion for consistency, I have.Clean and well-structured, the implementation is. Follows the established patterns for RangeXDDim1 and RangeXDDim2, it does. Yet, for better consistency with sibling implementations, a surfaces method could be beneficial.
Consider adding this implementation, you should:
py::class_<RangeXDDim3>(m, "RangeXDDim3") .def(py::init([](const std::array<double, 2u>& range0, const std::array<double, 2u>& range1, const std::array<double, 2u>& range2) { RangeXDDim3 range; range[0].shrink(range0[0], range0[1]); range[1].shrink(range1[0], range1[1]); range[2].shrink(range2[0], range2[1]); return range; })); + .def("surfaces", py::overload_cast<const RangeXDDim3&>( + &RangeXDDim3::surfaces, py::const_));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
Core/include/Acts/MagneticField/MultiRangeBField.hpp(1 hunks)Core/src/MagneticField/CMakeLists.txt(1 hunks)Core/src/MagneticField/MultiRangeBField.cpp(1 hunks)Examples/Python/src/Geometry.cpp(1 hunks)Examples/Python/src/MagneticField.cpp(2 hunks)Examples/Python/tests/test_magnetic_field.py(1 hunks)Tests/UnitTests/Core/MagneticField/CMakeLists.txt(1 hunks)Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp(1 hunks)docs/core/magnetic_field.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Core/src/MagneticField/MultiRangeBField.cpp
- Core/include/Acts/MagneticField/MultiRangeBField.hpp
- Core/src/MagneticField/CMakeLists.txt
- Tests/UnitTests/Core/MagneticField/CMakeLists.txt
- Examples/Python/src/MagneticField.cpp
🧰 Additional context used
🪛 Markdownlint (0.35.0)
docs/core/magnetic_field.md
248-248: Expected: asterisk; Actual: underscore
Emphasis style
(MD049, emphasis-style)
248-248: Expected: asterisk; Actual: underscore
Emphasis style
(MD049, emphasis-style)
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp
[error] 20-20: syntax error
(syntaxError)
🔇 Additional comments (2)
Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp (1)
20-20: Fix the syntax error in test case declaration, you must.
A syntax error in the test case declaration, I sense. Follow the BOOST test macro style guidelines, we should.
Apply this diff to correct the syntax:
-BOOST_AUTO_TEST_CASE(TestMultiRangeBField) {
+BOOST_AUTO_TEST_CASE(TestMultiRangeBField)
+{🧰 Tools
🪛 cppcheck (2.10-2)
[error] 20-20: syntax error
(syntaxError)
docs/core/magnetic_field.md (1)
238-259: More documentation details, require we do.
Clear the basic functionality is, but missing important details I sense:
- Performance characteristics compared to other implementations
- Cache implementation and its benefits
- Thread safety guarantees
Add these sections to documentation, you should:
corresponding field vector.
+### Performance and Implementation Details
+
+The implementation uses a simple caching mechanism to store the last matched
+region, providing improved performance for consecutive lookups within the same
+region. Compared to `ConstantBField`, this implementation has minimal overhead
+when using a single region, and scales linearly with the number of regions.
+
+### Thread Safety
+
+The `MultiRangeBField` is thread-safe when each thread uses its own cache
+instance obtained through `makeCache()`. The field configuration itself is
+immutable after construction.🧰 Tools
🪛 Markdownlint (0.35.0)
248-248: Expected: asterisk; Actual: underscore
Emphasis style
(MD049, emphasis-style)
248-248: Expected: asterisk; Actual: underscore
Emphasis style
(MD049, emphasis-style)
stephenswat
force-pushed
the
feat/multi_range_bfield
branch
from
b4c1434 to
954072f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp (1)
32-39: Well structured test cases, these are. Approve them, I do.Comprehensive coverage of test scenarios, you have achieved:
- Testing points inside first volume
- Testing points inside second volume
- Testing points in overlap region
- Testing points outside all volumes
Good use of BOOST_CHECK and BOOST_CHECK_CLOSE for assertions, I observe.
Consider adding more descriptive comments for each test block to explain the expected behavior in detail, you should.
Also applies to: 41-48, 50-57, 59-66, 68-72
Examples/Python/tests/test_magnetic_field.py (1)
77-112: Strong with the Force, these Python tests are.Well-structured test cases that mirror the C++ tests, you have created. Good practices observed:
- Testing error cases with pytest.raises
- Comprehensive field value verification
- Clear test structure
Add docstring to explain test purpose and scenarios, you should. Example:
def test_multiregion_bfield(): """Test MultiRangeBField behavior with overlapping regions. Test cases: - Invalid instantiation - Field values in first region - Field values in second region - Field values in overlap region """docs/core/magnetic_field.md (2)
238-244: Add context about ALICE3 use case, we must.Clear the basic functionality is, but strengthen the documentation with real-world example, we should. Add a note about ALICE3 use case, I suggest:
has its own constant magnetic field. This provides more flexibility than the {class}`Acts::ConstantBField` while providing higher performance than -{class}`Acts::InterpolatedBFieldMap`. +{class}`Acts::InterpolatedBFieldMap`. + +This implementation was primarily motivated by the ALICE3 experiment, where +different detector regions require distinct constant magnetic field values.
248-248: Consistent emphasis style, maintain we must.Replace underscore with asterisk for emphasis, according to project style:
-Magnetic field lookup then proceeds by finding the _last_ region in the +Magnetic field lookup then proceeds by finding the *last* region in the🧰 Tools
🪛 Markdownlint (0.35.0)
248-248: Expected: asterisk; Actual: underscore
Emphasis style(MD049, emphasis-style)
248-248: Expected: asterisk; Actual: underscore
Emphasis style(MD049, emphasis-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
Core/include/Acts/MagneticField/MultiRangeBField.hpp(1 hunks)Core/src/MagneticField/CMakeLists.txt(1 hunks)Core/src/MagneticField/MultiRangeBField.cpp(1 hunks)Examples/Python/src/Geometry.cpp(1 hunks)Examples/Python/src/MagneticField.cpp(2 hunks)Examples/Python/tests/test_magnetic_field.py(1 hunks)Tests/UnitTests/Core/MagneticField/CMakeLists.txt(1 hunks)Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp(1 hunks)docs/core/magnetic_field.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Tests/UnitTests/Core/MagneticField/CMakeLists.txt
- Core/src/MagneticField/CMakeLists.txt
- Examples/Python/src/MagneticField.cpp
- Core/src/MagneticField/MultiRangeBField.cpp
- Core/include/Acts/MagneticField/MultiRangeBField.hpp
🧰 Additional context used
🪛 Markdownlint (0.35.0)
docs/core/magnetic_field.md
248-248: Expected: asterisk; Actual: underscore
Emphasis style
(MD049, emphasis-style)
248-248: Expected: asterisk; Actual: underscore
Emphasis style
(MD049, emphasis-style)
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp
[error] 20-20: syntax error
(syntaxError)
🔇 Additional comments (4)
Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp (1)
20-20: Fix the syntax error in test case declaration, you must.
A syntax error in the test case declaration, there is. Match the existing review comment, this does.
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 20-20: syntax error
(syntaxError)
Examples/Python/src/Geometry.cpp (1)
495-508: Clean and consistent implementation, this is.
Follow the established pattern of RangeXDDim1 and RangeXDDim2, it does. Good practices observed:
- Proper use of pybind11 for Python bindings
- Consistent implementation pattern
- Clear parameter handling
Verify consistency with other dimensional implementations, we must:
✅ Verification successful
Verified, the consistency is. Approved, this implementation shall be.
Through the Force, seen the implementations I have. Perfect alignment exists:
- Each dimension (1,2,3) follows same pattern of initialization
- Consistent use of
shrink()method for range boundaries - Proper parameter handling with
std::array<double, 2u>
Most importantly, follows the established pattern perfectly, RangeXDDim3 does. A natural progression from RangeXDDim1 and RangeXDDim2, it represents.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check consistency across RangeXD implementations
rg -A 10 "RangeXDDim[123]" Examples/Python/src/Geometry.cpp
Length of output: 3191
docs/core/magnetic_field.md (2)
252-256: Well documented, the implementation details are.
Clear explanation of caching mechanism and thread safety guarantees, you provide. Addressed previous review concerns about documentation completeness, this has.
257-259: Properly integrated, the class reference is.
Follows the established documentation pattern, this does. Consistent with other class references in the document, it is.
stephenswat
force-pushed
the
feat/multi_range_bfield
branch
from
954072f to
45d2a4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp (3)
23-26: More descriptive comments for test regions, need we do.Clearer understanding of test regions, help others it would. Add comments explaining the physical meaning and purpose of each region's coordinates and field values.
inputs.emplace_back(RangeXD<3, double>{{0., 0., 0.}, {3., 3., 3.}}, Vector3{0., 0., 2.}); +// First region: Cubic volume from origin to (3,3,3) with 2T field in z-direction inputs.emplace_back(RangeXD<3, double>{{1., 1., 1.}, {2., 2., 10.}}, Vector3{2., 0., 0.}); +// Second region: Elongated volume with 2T field in x-direction, overlapping first region
34-38: A helper function for field verification, create we should.Repeated verification pattern, I see. A helper function, reduce duplication it would.
+namespace { +void checkField(const Result<Vector3>& r, double x, double y, double z) { + BOOST_CHECK(r.ok()); + BOOST_CHECK_CLOSE((*r)[0], x, 0.05); + BOOST_CHECK_CLOSE((*r)[1], y, 0.05); + BOOST_CHECK_CLOSE((*r)[2], z, 0.05); +} +} // namespace + Result<Vector3> r = bfield.getField({0.5, 0.5, 0.5}, bcache); -BOOST_CHECK(r.ok()); -BOOST_CHECK_CLOSE((*r)[0], 0., 0.05); -BOOST_CHECK_CLOSE((*r)[1], 0., 0.05); -BOOST_CHECK_CLOSE((*r)[2], 2., 0.05); +checkField(r, 0., 0., 2.);
60-66: Document overlapping region behavior, we must.Clear the behavior is, but explicit documentation missing it is. Add comment explaining precedence rules.
- // Test a point inside the second volume in the overlap region. + // Test a point inside the overlap region between volumes. + // Expect field from second volume (2T in x-direction) as it appears later in the list, + // demonstrating the last-match-wins behavior for overlapping regions. { Result<Vector3> r = bfield.getField({1.5, 1.5, 1.5}, bcache);Examples/Python/tests/test_magnetic_field.py (2)
78-79: More Pythonic assertions, use we should.The force of Python's context manager, strong it is. Better error message, provide we must.
- with pytest.raises(TypeError): - acts.MultiRangeBField() + with pytest.raises(TypeError, match=".*at least 1 argument.*"): + acts.MultiRangeBField()
94-112: A helper function for field assertions, create we must.Repeated assertions, I see. DRY principles, follow we should.
+def assert_field_vector(rv, x, y, z): + """Assert that field vector components match expected values.""" + assert rv[0] == pytest.approx(x) + assert rv[1] == pytest.approx(y) + assert rv[2] == pytest.approx(z) + rv = f.getField(acts.Vector3(0.5, 0.5, 0.5), fc) -assert rv[0] == pytest.approx(0.0) -assert rv[1] == pytest.approx(0.0) -assert rv[2] == pytest.approx(2.0) +assert_field_vector(rv, 0.0, 0.0, 2.0)docs/core/magnetic_field.md (1)
238-259: Enhanced documentation with examples and performance details, add we must.Clear the documentation is, but stronger with examples and performance characteristics it would be.
Add these sections to documentation:
field configuration itself is immutable after construction. + +#### Example Configuration + +```cpp +std::vector<std::pair<Acts::RangeXD<3, double>, Acts::Vector3>> regions; +// Define a 2T field in z-direction for a cubic volume +regions.emplace_back( + Acts::RangeXD<3, double>{{0., 0., 0.}, {3., 3., 3.}}, + Acts::Vector3{0., 0., 2.}); +// Define a 2T field in x-direction for an overlapping volume +regions.emplace_back( + Acts::RangeXD<3, double>{{1., 1., 1.}, {2., 2., 10.}}, + Acts::Vector3{2., 0., 0.}); +Acts::MultiRangeBField bfield(std::move(regions)); +``` + +#### Performance Characteristics + +The implementation has O(n) complexity for field lookup, where n is the number +of regions. However, the caching mechanism provides O(1) lookup for consecutive +queries within the same region, making it efficient for typical use cases where +field queries follow continuous trajectories.🧰 Tools
🪛 Markdownlint (0.35.0)
248-248: Expected: asterisk; Actual: underscore
Emphasis style(MD049, emphasis-style)
248-248: Expected: asterisk; Actual: underscore
Emphasis style(MD049, emphasis-style)
Examples/Python/src/Geometry.cpp (1)
495-508: Approve the changes, I do. Hmmmm.Well-implemented, the Python bindings for
RangeXDDim3are. Follows the established patterns of dimension 1 and 2, it does. Consistent with the Force, your code is.A small suggestion, I have. For consistency with
RangeXDDim1andRangeXDDim2, consider adding thesurfacesmethod binding, you should:py::class_<RangeXDDim3>(m, "RangeXDDim3") .def(py::init([](const std::array<double, 2u>& range0, const std::array<double, 2u>& range1, const std::array<double, 2u>& range2) { RangeXDDim3 range; range[0].shrink(range0[0], range0[1]); range[1].shrink(range1[0], range1[1]); range[2].shrink(range2[0], range2[1]); return range; })); + .def("surfaces", py::overload_cast<const RangeXDDim3&>( + &KdtSurfacesDim3Bin100::surfaces, py::const_));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
Core/include/Acts/MagneticField/MultiRangeBField.hpp(1 hunks)Core/src/MagneticField/CMakeLists.txt(1 hunks)Core/src/MagneticField/MultiRangeBField.cpp(1 hunks)Examples/Python/src/Geometry.cpp(1 hunks)Examples/Python/src/MagneticField.cpp(2 hunks)Examples/Python/tests/test_magnetic_field.py(1 hunks)Tests/UnitTests/Core/MagneticField/CMakeLists.txt(1 hunks)Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp(1 hunks)docs/core/magnetic_field.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Core/src/MagneticField/CMakeLists.txt
- Tests/UnitTests/Core/MagneticField/CMakeLists.txt
- Examples/Python/src/MagneticField.cpp
- Core/src/MagneticField/MultiRangeBField.cpp
- Core/include/Acts/MagneticField/MultiRangeBField.hpp
🧰 Additional context used
🪛 Markdownlint (0.35.0)
docs/core/magnetic_field.md
248-248: Expected: asterisk; Actual: underscore
Emphasis style
(MD049, emphasis-style)
248-248: Expected: asterisk; Actual: underscore
Emphasis style
(MD049, emphasis-style)
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp
[error] 20-20: syntax error
(syntaxError)
stephenswat
force-pushed
the
feat/multi_range_bfield
branch
from
45d2a4c to
46f5666
Compare
In order to facilitate the ALICE3 use-case (as described to me by @Cas1997) we need to support magnetic fields that are constant in multiple regions, but which may have different field values in each of these regions. I implemented a new magnetic field provider based on a mapping of 3D ranges to field vectors which should support this use case. The core idea is that the user provides a list of regions and the provider will check (with some simple caching) which region matches. The mechanism implemented is that -- in case of overlap -- the region later in the list takes precedence. Also implements C++ and Python tests for this new field provider.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Examples/Python/tests/test_magnetic_field.py (1)
77-112: Add test case for point outside all regions, you should.Currently, no test for points outside defined volumes, there is. Ensure correct behavior when position is outside all ranges, you must.
Consider adding this test:
rv = f.getField(acts.Vector3(-1.0, -1.0, -1.0), fc) assert rv is None or rv == pytest.approx(acts.Vector3(0.0, 0.0, 0.0))docs/core/magnetic_field.md (1)
248-248: Use asterisks for emphasis, you should.Markdownlint advises replacing underscores with asterisks for emphasis style consistency.
Apply this diff to fix the emphasis style:
-Magnetic field lookup then proceeds by finding the _last_ region in the +Magnetic field lookup then proceeds by finding the *last* region in the🧰 Tools
🪛 Markdownlint (0.35.0)
248-248: Expected: asterisk; Actual: underscore
Emphasis style(MD049, emphasis-style)
248-248: Expected: asterisk; Actual: underscore
Emphasis style(MD049, emphasis-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
Core/include/Acts/MagneticField/MultiRangeBField.hpp(1 hunks)Core/src/MagneticField/CMakeLists.txt(1 hunks)Core/src/MagneticField/MultiRangeBField.cpp(1 hunks)Examples/Python/src/Geometry.cpp(1 hunks)Examples/Python/src/MagneticField.cpp(2 hunks)Examples/Python/tests/test_magnetic_field.py(1 hunks)Tests/UnitTests/Core/MagneticField/CMakeLists.txt(1 hunks)Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp(1 hunks)docs/core/magnetic_field.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Tests/UnitTests/Core/MagneticField/CMakeLists.txt
- Core/src/MagneticField/CMakeLists.txt
- Examples/Python/src/MagneticField.cpp
- Core/include/Acts/MagneticField/MultiRangeBField.hpp
🧰 Additional context used
🪛 Markdownlint (0.35.0)
docs/core/magnetic_field.md
248-248: Expected: asterisk; Actual: underscore
Emphasis style
(MD049, emphasis-style)
248-248: Expected: asterisk; Actual: underscore
Emphasis style
(MD049, emphasis-style)
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp
[error] 20-20: syntax error
(syntaxError)
🔇 Additional comments (3)
Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp (2)
36-38: Review the tolerance value in BOOST_CHECK_CLOSE, you should.
The tolerance is set to 0.05, representing 0.05%. Perhaps 5.0% tolerance intended, you did. Adjust accordingly, if necessary.
Also applies to: 45-47, 54-56, 63-65
20-20:
Syntax error at line 20, there is.
Static analysis reports a syntax error in the test case declaration. Verify the BOOST test macro and ensure proper syntax, you must.
Apply this diff to correct the syntax:
-BOOST_AUTO_TEST_CASE(TestMultiRangeBField) {
+BOOST_AUTO_TEST_CASE(TestMultiRangeBField)
+{🧰 Tools
🪛 cppcheck (2.10-2)
[error] 20-20: syntax error
(syntaxError)
Examples/Python/src/Geometry.cpp (1)
495-508: Successfully exposed RangeXDDim3 to Python, you have.
Enhances functionality by allowing 3D ranges in Python bindings, it does.
|
resolved the AI conversations to make this pass |
|
acts-project-service
added
the
Breaks Athena build
In order to facilitate the ALICE3 use-case (as described to me by @Cas1997) we need to support magnetic fields that are constant in multiple regions, but which may have different field values in each of these regions. I implemented a new magnetic field provider based on a mapping of 3D ranges to field vectors which should support this use case.
The core idea is that the user provides a list of regions and the provider will check (with some simple caching) which region matches. The mechanism implemented is that -- in case of overlap -- the region later in the list takes precedence.
Also implements C++ and Python tests for this new field provider.
Summary by CodeRabbit
Release Notes
New Features
MultiRangeBFieldclass for modeling magnetic fields with multiple overlapping regions.RangeXDDim3class for handling three-dimensional ranges in the geometry module.Tests
MultiRangeBFieldclass, covering various scenarios to validate its behavior.acts.MultiRangeBFieldclass in Python.Documentation
Acts::MultiRangeBFieldclass and its functionality, including caching mechanisms.Chores
MultiRangeBFieldclass.MultiRangeBFieldclass to the project.