feat: new segmentation class#4237
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughMuch change, this pull request brings. The Fatras digitization module receives new segmentation capabilities, introducing a CartesianSegmentation class and interface for 2D segmentation. Channelizer and PlanarSurfaceDrift interfaces are extended, with error handling for drift conditions improved. Axis interfaces in Core are updated, adding pure virtual binning methods. Test coverage expands, with new and enhanced tests for segmentation and planar drift, and build scripts updated to compile new sources. Implementation details for segmentation and channelization are refactored, centralizing logic and improving modularity. Error enums and messages gain new entries for drift-related failures. Changes
Sequence Diagram(s)sequenceDiagram
participant Hit
participant Channelizer
participant PlanarSurfaceDrift
participant SurfaceMask
participant Segmentizer
participant Segmentation
Hit->>Channelizer: channelize(hit, surface, gctx, drift, binutil, thickness, minRelPerpDrift)
Channelizer->>PlanarSurfaceDrift: toReadout(gctx, surface, thickness, pos, dir, drift)
PlanarSurfaceDrift-->>Channelizer: Result<(drifted2D, full3D)>
Channelizer->>SurfaceMask: apply(drifted2D)
SurfaceMask-->>Channelizer: masked2D
Channelizer->>Segmentizer: segments(masked2D, binutil)
Segmentizer->>Segmentation: channelSteps(start, end)
Segmentation-->>Segmentizer: steps
Segmentizer-->>Channelizer: channel segments
Channelizer-->>Hit: vector<ChannelSegment>
Suggested labels
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 6
🧹 Nitpick comments (21)
Core/include/Acts/Utilities/IAxis.hpp (1)
69-72: A minor documentation style inconsistency, I sense.Mmm, notice do I that the documentation style differs slightly. While the first comment begins with
@brief, this one does not. Consistent in documentation style, we should be.- // Get the bin center of the given bin + /// @brief Get the bin center of the given binTests/UnitTests/Fatras/Digitization/SegmentationTests.cpp (1)
23-35: Test case for CartesianSegmentation, thorough it must be. Clarify comments, you should.Created the test correctly, you have, but unclear comments I sense. "0 is underflow bin" - explain better this should. Expected position (-0.5, -0.5) - explain why this value is expected, you must.
Improve the comments with explanation like this, you could:
- // Test binning - 0 is underflow bin + // Test binning - Bin indices are 1-based with 0 reserved for underflow + // Point (0.1, 0.1) should be in bin 10 for x-axis and bin 12 for y-axis auto bin = segmentation.bin(Vector2(0.1, 0.1)); BOOST_CHECK_EQUAL(bin[0], 10); BOOST_CHECK_EQUAL(bin[1], 12); - // Test position + // Test position - Expected center of bin (10,12) is (-0.5,-0.5) auto pos = segmentation.position(bin); CHECK_CLOSE_ABS(pos.x(), -0.5, 1e-5); CHECK_CLOSE_ABS(pos.y(), -0.5, 1e-5);Fatras/include/ActsFatras/Digitization/Channelizer.hpp (2)
11-13: Include only what you use, hmmm?In this header no direct use of
BinUtilityI see; yet heavy include it still does. Compile time, it will slow. Forward-declare or move include to the.cppfile you should.
42-50: Default drift threshold hard-codes, perilous it is.
0.001a magic number remains. Explain withconstexprnamed constant or documentation comment, you should. So future maintainers the Force will guide.Fatras/src/Digitization/Segmentizer.cpp (1)
44-56: Segmentation object each call you forge; costly, it may be.
CartesianSegmentationconstructed for every hit. For many hits on same surface, allocations pile up they will. Cache per‐surface instance or pass pre-built segmentation in, consider you should.Core/include/Acts/Utilities/Axis.hpp (4)
310-313: Override keyword, forgotten it was.
finalimplies override, yet explicitoverride finalclearer it is, and compiler warnings it quiets. Add, you should.- std::size_t getBin(double x) const final { + std::size_t getBin(double x) const final override {
355-357: Consistency in declaration, maintain you must.Likewise for
getBinCenter, addoverridekeyword.- double getBinCenter(std::size_t bin) const final { + double getBinCenter(std::size_t bin) const final override {
622-626: Variable axis: same song, second verse.
getBinshould mirror change.- std::size_t getBin(double x) const final { + std::size_t getBin(double x) const final override {
672-674: Variable axis: finish the quartet, yes.For
getBinCenterapply too.- double getBinCenter(std::size_t bin) const final { + double getBinCenter(std::size_t bin) const final override {Fatras/src/Digitization/Channelizer.cpp (2)
27-29: Typos, clarity cloud they do.
fullSegementa miss-spelling is, hmmm. Readability it hurts and consistency breaks. Rename tofullSegment, you should.- const auto& [driftedSegment, fullSegement] = atReadoutPlane.value(); + const auto& [driftedSegment, fullSegment] = atReadoutPlane.value(); ... - double fullPathLength = (fullSegement[1] - fullSegement[0]).norm(); + double fullPathLength = (fullSegment[1] - fullSegment[0]).norm();
49-54: Variable name, muddled it is.
sclale2Dto3Dthe eye confuses.scale2Dto3Dcall it, clear the Force will be.- double sclale2Dto3D = fullPathLength / driftedPathLength; + double scale2Dto3D = fullPathLength / driftedPathLength; ... - segment.activation *= sclale2Dto3D; + segment.activation *= scale2Dto3D;Fatras/include/ActsFatras/Digitization/PlanarSurfaceDrift.hpp (1)
42-48: Docs and spelling, polished they must be.
alwaywsand trailing slash/the reader distract. Also parameter spelleddriftdirhere, yetdriftDirin the cpp it is. Align names you should, clarity it brings.Fatras/src/Digitization/PlanarSurfaceDrift.cpp (1)
41-47: Large driftScale, potential imbalance I sense.
driftScale = 0.5 * thickness / driftDir.z();
ShoulddriftDir.z()tiny positive but just aboves_epsilon, enormous shift you create, detector geometry exceed it may. Clamp or early-return with error, consider.Ask you I must: Verify maximal drift values foreseen are, or protection add.
Tests/UnitTests/Fatras/Digitization/PlanarSurfaceDriftTests.cpp (2)
120-200: Heavy file I/O in unit tests, unwise this is.Many
.objfiles the test writes. CI environments slow it makes, filesystem permissions sometimes deny. Mock or guard such output behind debug flag you should, or remove.- std::ofstream fiout; - fiout.open("PlanarSurfaceDrift_segment_orig_3D.obj"); + #ifdef ACTS_DEBUG_OUTPUT + std::ofstream fiout("PlanarSurfaceDrift_segment_orig_3D.obj"); + #endif ... - std::ofstream fsout; - fsout.open("PlanarSurfaceDrift_" + surfaceNames[isf] + ".obj"); + #ifdef ACTS_DEBUG_OUTPUT + std::ofstream fsout("PlanarSurfaceDrift_" + surfaceNames[isf] + ".obj"); + #endifSimilar guards around all file writing apply.
50-70: Unused bindings, compile warnings spawn they may.
oSegment,dScale1variables unused are. Replace withstd::ignoreor omit they should be to silence warnings.-auto [noDriftSegment, oSegment] = ... +auto [noDriftSegment, std::ignore] = ...Fatras/src/Digitization/Segmentation.cpp (5)
11-11: Remove unused include, you must.
<iostream>included is, but used it is not. Remove unnecessary includes we should, to reduce compilation time and dependencies.-#include <iostream>
13-15: Unnecessary base class initialization, I see.Explicitly initializing
ISegmentation()needed it is not. Default constructor of abstract base class automatically called it will be.ActsFatras::CartesianSegmentation::CartesianSegmentation( const Acts::ProtoAxis& xAxis, const Acts::ProtoAxis& yAxis) - : ISegmentation(), m_xAxis(xAxis), m_yAxis(yAxis) {} + : m_xAxis(xAxis), m_yAxis(yAxis) {}
46-46: Spelling error in comment, fix you should.Typo in comment there is. "The're change" written is, when "There's a change" it should be.
- // The're change in the x direction + // There's a change in the x direction
61-61: Unclear comment, improve you should.Comment "The lines channel segment lines along y" confusing it is. Clarity we need.
- // The lines channel segment lines along y + // Handle channel segments crossing y-axis boundaries
74-75: Sort the channel steps, consider you should.The
ChannelStepstruct has a comparison operator for path-based sorting, but steps not sorted are. For proper ordering by path length, sort before returning you should.+ // Sort channel steps by path length + std::sort(cSteps.begin(), cSteps.end()); return cSteps;Also, include the necessary header:
+#include <algorithm>Fatras/include/ActsFatras/Digitization/Segmentation.hpp (1)
25-25: Typo in comment, fix you must."patlength" written is, when "path length" it should be.
- /// The patlength from the start + /// The path length from the start
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
Core/include/Acts/Utilities/Axis.hpp(4 hunks)Core/include/Acts/Utilities/IAxis.hpp(1 hunks)Fatras/CMakeLists.txt(1 hunks)Fatras/include/ActsFatras/Digitization/Channelizer.hpp(2 hunks)Fatras/include/ActsFatras/Digitization/DigitizationError.hpp(1 hunks)Fatras/include/ActsFatras/Digitization/PlanarSurfaceDrift.hpp(2 hunks)Fatras/include/ActsFatras/Digitization/Segmentation.hpp(1 hunks)Fatras/include/ActsFatras/Digitization/Segmentizer.hpp(1 hunks)Fatras/src/Digitization/Channelizer.cpp(1 hunks)Fatras/src/Digitization/DigitizationError.cpp(1 hunks)Fatras/src/Digitization/PlanarSurfaceDrift.cpp(2 hunks)Fatras/src/Digitization/Segmentation.cpp(1 hunks)Fatras/src/Digitization/Segmentizer.cpp(3 hunks)Tests/UnitTests/Core/Navigation/NavigationStateUpdatersTests.cpp(1 hunks)Tests/UnitTests/Fatras/Digitization/CMakeLists.txt(1 hunks)Tests/UnitTests/Fatras/Digitization/PlanarSurfaceDriftTests.cpp(2 hunks)Tests/UnitTests/Fatras/Digitization/SegmentationTests.cpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
Core/include/Acts/Utilities/IAxis.hpp (3)
Tests/UnitTests/Core/Navigation/NavigationStateUpdatersTests.cpp (4)
x(130-130)x(130-130)bin(132-134)bin(132-132)Core/include/Acts/Utilities/Axis.hpp (22)
x(310-313)x(310-310)x(381-381)x(381-381)x(622-626)x(622-622)x(698-700)x(698-698)bin(268-272)bin(268-269)bin(280-284)bin(280-281)bin(292-298)bin(292-293)bin(330-332)bin(330-330)bin(344-346)bin(344-344)bin(355-357)bin(355-355)bin(580-584)bin(580-581)Core/include/Acts/Geometry/SurfaceArrayCreator.hpp (2)
x(69-80)x(69-69)
Fatras/include/ActsFatras/Digitization/Channelizer.hpp (1)
Fatras/include/ActsFatras/Digitization/PlanarSurfaceDrift.hpp (1)
gctx(52-55)
Fatras/src/Digitization/Channelizer.cpp (1)
Fatras/include/ActsFatras/Digitization/Channelizer.hpp (1)
hit(45-49)
Fatras/src/Digitization/Segmentation.cpp (3)
Fatras/include/ActsFatras/Digitization/Segmentation.hpp (8)
CartesianSegmentation(73-74)CartesianSegmentation(75-75)bin(59-60)bin(81-81)pos(51-51)pos(78-78)start(67-68)start(84-85)Core/include/Acts/Utilities/IAxis.hpp (2)
bin(72-72)x(67-67)Core/include/Acts/Utilities/Axis.hpp (19)
bin(268-272)bin(268-269)bin(280-284)bin(280-281)bin(292-298)bin(292-293)bin(330-332)bin(330-330)bin(344-346)bin(344-344)bin(355-357)x(310-313)x(310-310)x(381-381)x(381-381)x(622-626)x(622-622)x(698-700)x(698-698)
🪛 Cppcheck (2.10-2)
Tests/UnitTests/Fatras/Digitization/SegmentationTests.cpp
[error] 17-17: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
Tests/UnitTests/Fatras/Digitization/PlanarSurfaceDriftTests.cpp
[error] 29-29: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: CI Bridge / build_gnn_tensorrt
- GitHub Check: merge-sentinel
🔇 Additional comments (12)
Core/include/Acts/Utilities/IAxis.hpp (1)
65-67: Add pure virtual methods to IAxis, a good decision this is!Strong with the Force, this interface addition is. The
getBinmethod, helpful it will be, for coordinate to bin index conversion. The binning logic, crucial for segmentation APIs it is.Fatras/src/Digitization/DigitizationError.cpp (1)
34-35: Properly handle the new DriftError, you do.Mmm, well implemented this error case is! Consistent with other error messages, your implementation remains. Simple yet informative, the message is, yes.
Tests/UnitTests/Fatras/Digitization/CMakeLists.txt (1)
8-8: Tests for new functionality, essential they are!Wise you are, to include tests for the new segmentation implementation. Without tests, in darkness we would be. The naming convention, consistent it is with other test targets, hmm.
Fatras/include/ActsFatras/Digitization/DigitizationError.hpp (2)
22-22: Add DriftError enum value, you have. Good, this is.To the Force, balance you bring by adding this error type. Detect drift-related issues, you now can. Clear and descriptive, the name is.
16-23:Details
✅ Verification successful
Verify the DriftError implementation, we must.
The path to wisdom requires validation, yes. Make sure properly handled this error is, in code that uses it.
🏁 Script executed:
#!/bin/bash # Find where DriftError is being used in the codebase rg "DriftError" --type cppLength of output: 940
I seek clarity on DriftError’s handling in two places—PlanarSurfaceDrift.cpp and DigitizationError.cpp. Let us inspect them:
🏁 Script executed:
#!/bin/bash # Inspect return of DriftError in PlanarSurfaceDrift.cpp rg "DigitizationError::DriftError" -C3 Fatras/src/Digitization/PlanarSurfaceDrift.cpp # Inspect case handling of DriftError in DigitizationError.cpp rg "case DigitizationError::DriftError" -C3 Fatras/src/Digitization/DigitizationError.cppLength of output: 780
DriftError handling verified, properly implemented it is.
The
DriftErrorreturn in PlanarSurfaceDrift.cpp and its mapping to “Drift error occurred.” in DigitizationError.cpp cover the error case as intended.Fatras/CMakeLists.txt (1)
4-4: New source files, added correctly you have.Noticed the addition of Channelizer.cpp and Segmentation.cpp to the build, I have. Good alignment with the PR objectives, this shows. New segmentation API, these files implement, mmm.
Also applies to: 8-9
Fatras/include/ActsFatras/Digitization/Segmentizer.hpp (1)
13-13: Include path, properly added it is.Appropriate is this include of the new Segmentation.hpp header. Enables the extraction of the ChannelStep functionality from this file, it does. Wise refactoring, yes, mmmm.
Tests/UnitTests/Core/Navigation/NavigationStateUpdatersTests.cpp (1)
130-134: Implemented the required interface methods, you have. Good, this is.The IAxis interface methods, correctly implemented they are. For getBin, returns 1 for negative values and 2 for non-negative values, it does. For getBinCenter, calculates center position of bin correctly, it does. Mark these as final, you wisely did, mmm.
Tests/UnitTests/Fatras/Digitization/SegmentationTests.cpp (1)
17-17: Test suite setup, correct it is.Ignore the static analysis warning about unknown macro BOOST_AUTO_TEST_SUITE. Standard Boost.Test macro this is, mmm. False positive from static analyzer, it certainly is.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 17-17: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
Fatras/src/Digitization/Segmentizer.cpp (3)
17-18: New segmentation header included, good this is.Clarity and separation the change brings; concern none have I.
55-57: Out-of-range bins handled, ensure you must.When
cSegmentation.bin(start)returns{-1,-1}for under/overflow, undefined behaviour follow it might. Guard or document, wiser it is.
130-133:lastDeltatype modernized, harmony I sense.
std::array<int,2>clear and safe; approve I do.
github-actions
bot
added
Component - Core
github-actions
bot
added
Component - Examples
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@asalzburger should we get this in? I think there were some output changes that I'm not sure we were expecting, an error running the code coverage calculation (this might go away if you rebase) and a few clang-tidy warnings. |
2 participants
This PR introduces a new segmentation API, which will allow users to define their own segmentation as long as it does channelize correctly.
It also switches internally to this (and uses the grid instead of BinUtility).
It will be followed up by an API changing PR which will remove BinUtility from the Digitisation at all.
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor