-
Notifications
You must be signed in to change notification settings - Fork 184
feat: simpler 2d/3d calculation #4169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: simpler 2d/3d calculation #4169
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 WalkthroughChanged, the code has been. In the Changes
Possibly related PRs
Suggested labels
Poem
🪧 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Fatras/include/ActsFatras/Digitization/PlanarSurfaceDrift.hpp (1)
45-45
: Struct instead of tuple, consider you might.
Improve readability and clarity, a named struct would, hmm?- std::tuple<Segment2D, double> toReadout(...) + struct ReadoutResult { + Segment2D segment; + double driftScale; + }; + ReadoutResult toReadout(...)Tests/UnitTests/Fatras/Digitization/PlanarSurfaceDriftTests.cpp (1)
48-49
: Test the drift scale, you could.
UnpackeddScale
variables you have, yet unverified they are. Hmmm, coverage for new drift scale lacking, it is.Add checks for
dScale
, you can, to ensure correctness of the returned scaling factor.Also applies to: 61-62, 64-64, 66-66, 75-76, 78-88, 91-91, 97-98, 100-110
Fatras/include/ActsFatras/Digitization/Channelizer.hpp (1)
41-55
: Rename 'dScalor', consider you should.
"scalor" correct English, it is not. "scaleFactor" or "driftScale," more descriptive it is.- auto [ driftedSegment, dScalor ] = m_surfaceDrift.toReadout(...) + auto [ driftedSegment, driftScale ] = m_surfaceDrift.toReadout(...) ... - [dScalor](auto& seg) { seg.activation *= dScalor; } + [driftScale](auto& seg) { seg.activation *= driftScale; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Fatras/include/ActsFatras/Digitization/Channelizer.hpp
(1 hunks)Fatras/include/ActsFatras/Digitization/PlanarSurfaceDrift.hpp
(2 hunks)Fatras/src/Digitization/PlanarSurfaceDrift.cpp
(1 hunks)Tests/UnitTests/Fatras/Digitization/PlanarSurfaceDriftTests.cpp
(4 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
Tests/UnitTests/Fatras/Digitization/PlanarSurfaceDriftTests.cpp (2)
Core/include/Acts/EventData/MultiComponentTrackParameters.hpp (6)
geoCtx
(73-109)geoCtx
(73-76)geoCtx
(214-216)geoCtx
(214-214)geoCtx
(222-224)geoCtx
(222-222)Core/include/Acts/EventData/GenericBoundTrackParameters.hpp (8)
geoCtx
(59-76)geoCtx
(59-64)geoCtx
(220-226)geoCtx
(220-220)geoCtx
(236-238)geoCtx
(236-236)geoCtx
(286-288)geoCtx
(286-286)
Fatras/include/ActsFatras/Digitization/Channelizer.hpp (1)
Fatras/include/ActsFatras/Digitization/PlanarSurfaceDrift.hpp (1)
gctx
(45-48)
Fatras/src/Digitization/PlanarSurfaceDrift.cpp (1)
Fatras/include/ActsFatras/Digitization/PlanarSurfaceDrift.hpp (1)
gctx
(45-48)
🔇 Additional comments (2)
Fatras/include/ActsFatras/Digitization/PlanarSurfaceDrift.hpp (1)
16-16
: Includes correct, they are.
Add the header for tuples you did, correct it is.Fatras/src/Digitization/PlanarSurfaceDrift.cpp (1)
15-21
: Parameter marked 'maybe_unused,' consistent it seems.
A new signature with two values returned, correct for your design, it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (7)
Tests/UnitTests/Fatras/Digitization/PlanarSurfaceDriftTests.cpp (2)
23-25
: File operations for visualization, added you have.For debugging and demonstrating drift behaviors, visual output helpful is. Remember that in CI environments, write permissions not guaranteed are.
Consider adding error handling for file operations or making file paths configurable, you should.
123-160
: File operations scattered throughout test, refactoring opportunity this is.Repeated file output operations, see I do. A helper function to encapsulate file writing logic, improve readability it would.
Consider refactoring repeated file operations into helper methods:
+ // Helper to write vertex data to file + auto writeVertex = [](std::ofstream& out, const Acts::Vector3& vertex) -> void { + out << "v " << vertex.x() << " " << vertex.y() << " " << vertex.z() << "\n"; + }; + + // Helper to write surface to file + auto writeSurfaceToFile = [&](const std::string& filename, + const std::shared_ptr<Acts::Surface>& sf) -> void { + std::ofstream fsout; + fsout.open(filename); + auto polyHedron = sf->polyhedronRepresentation(tContext); + for (const auto& vertex : polyHedron.vertices) { + writeVertex(fsout, vertex); + } + for (const auto& face : polyHedron.faces) { + fsout << "f"; + for (const auto& vertex : face) { + fsout << " " << vertex + 1; + } + fsout << "\n"; + } + fsout.close(); + };Fatras/include/ActsFatras/Digitization/Channelizer.hpp (3)
41-41
: Typo in comment, fixed it should be.Small typo "Drfted" instead of "Drifted" in comment, I see.
- // Drfted surface and scalor 2D to 3D segment + // Drifted surface and scalar 2D to 3D segment
48-48
: Tuple unpacking implemented, but typo exists.Variable name "fullSegement" misspelled, corrected it should be.
- auto [driftedSegment, fullSegement] = drifted.value(); + auto [driftedSegment, fullSegment] = drifted.value();
69-74
: Simplified scaling calculation, more direct approach this is.Typo in variable name "sclale2Dto3D", fixed it should be. The scaling logic itself, simplified and clearer now it is. Directly uses 3D and 2D path lengths without complex calculations.
- double sclale2Dto3D = fullPathLength / driftedPathLength; + double scale2Dto3D = fullPathLength / driftedPathLength; - segment.activation *= sclale2Dto3D; + segment.activation *= scale2Dto3D;Fatras/src/Digitization/PlanarSurfaceDrift.cpp (1)
16-17
: Return a tuple, flexible approach it is.But clearer than a struct, the tuple may be less. Consider a custom struct, you could, to name the returned segments.
Fatras/include/ActsFatras/Digitization/PlanarSurfaceDrift.hpp (1)
44-48
: Typo “alwayws,” fix we must.Replace with “always,” you should.
- /// @note The readout is alwayws emulated + /// @note The readout is always emulated
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Fatras/include/ActsFatras/Digitization/Channelizer.hpp
(1 hunks)Fatras/include/ActsFatras/Digitization/DigitizationError.hpp
(1 hunks)Fatras/include/ActsFatras/Digitization/PlanarSurfaceDrift.hpp
(2 hunks)Fatras/src/Digitization/DigitizationError.cpp
(1 hunks)Fatras/src/Digitization/PlanarSurfaceDrift.cpp
(2 hunks)Tests/UnitTests/Fatras/Digitization/PlanarSurfaceDriftTests.cpp
(2 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
Tests/UnitTests/Fatras/Digitization/PlanarSurfaceDriftTests.cpp (3)
Core/include/Acts/EventData/MultiComponentTrackParameters.hpp (6)
geoCtx
(73-109)geoCtx
(73-76)geoCtx
(214-216)geoCtx
(214-214)geoCtx
(222-224)geoCtx
(222-222)Core/include/Acts/EventData/GenericBoundTrackParameters.hpp (8)
geoCtx
(59-76)geoCtx
(59-64)geoCtx
(220-226)geoCtx
(220-220)geoCtx
(236-238)geoCtx
(236-236)geoCtx
(286-288)geoCtx
(286-286)Tests/UnitTests/Core/Seeding/PathSeederTest.cpp (2)
geoCtx
(61-91)geoCtx
(61-63)
Fatras/include/ActsFatras/Digitization/Channelizer.hpp (1)
Fatras/include/ActsFatras/Digitization/PlanarSurfaceDrift.hpp (1)
gctx
(51-54)
🪛 Cppcheck (2.10-2)
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 (15)
- GitHub Check: CI Bridge / build_gnn_tensorrt
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2404_clang19
- GitHub Check: CI Bridge / linux_ubuntu_2204
- 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: merge-sentinel
- GitHub Check: missing_includes
- GitHub Check: build_debug
🔇 Additional comments (26)
Fatras/include/ActsFatras/Digitization/DigitizationError.hpp (1)
22-22
: Wise addition to error types, this is.New
DriftError
enumerator properly added, I see. Consistent with existing code style, it is. For drift-related failures, clear error reporting provides, hmm.Fatras/src/Digitization/DigitizationError.cpp (1)
34-35
: Proper error message, you have provided.Clear and concise message for the new
DriftError
enumerator, this is. Consistent with other error messages in the switch statement, yes. Help developers debug drift-related issues, it will.Tests/UnitTests/Fatras/Digitization/PlanarSurfaceDriftTests.cpp (7)
16-16
: New include for RectangleBounds, necessary it is.For the enhanced tests that use rectangle bounds for surfaces, required this is. Good practice to include only what is needed, you follow.
19-19
: Enumerate utility, useful for tests it is.The Enumerate wrapper, simplify iteration with indices it does. Clean and readable code, produces this will.
31-31
: Renamed test case, clearer intent it shows.From
PlanarSurfaceDrift
toPlanarSurfaceDriftSimpleTests
renamed, better reflects the test scope this does. Good practice when test suite expands, yes.
50-54
: Return value unpacking, adaptation to new API this is.Now a tuple with both drifted segment and original segment,
toReadout
returns. Structured binding, elegant way to unpack the result it is. Consistent with changes in Channelizer.hpp, this remains.
64-68
: Similar unpacking pattern, consistent throughout the test.The pattern of unpacking the tuple from
toReadout
into two variables, maintained it is. Good for code consistency and readability, this practice is.
75-243
: Comprehensive enhanced tests, thorough validation they provide.Extensive test case for drift functionality, created you have. Multiple scenarios it tests:
- Entry, readout, and exit surface creation and handling
- Different drift scenarios with visual outputs
- Validation of drift calculations and transformations
- Special case testing for particle normal to surface
- Error case testing for invalid particle directions
Thorough testing of complex functionality, important this is. May the path to quality code, clear it now becomes.
246-258
: Error testing, complete your approach is.Good to see both success and error cases tested, hmm. Validates error handling for edge cases, this does. The full lifecycle of the API, covered it now is.
Fatras/include/ActsFatras/Digitization/Channelizer.hpp (2)
42-46
: Error handling added, robust code this makes.Error forwarding from
toReadout
call, good practice this is. Fails early if drift operation unsuccessful, preventing cascade of errors, yes.
60-67
: Special case handling for near-zero path lengths, good this is.When drifted path length very small, numerical stability ensured by setting fixed activation. Prevents division by zero in scaling calculation, wise approach this is.
Fatras/src/Digitization/PlanarSurfaceDrift.cpp (7)
12-12
: Include header, correct this is.Yes, needed for
DriftError
usage, it is.
26-27
: Local transformations, done well they are.Yes, transform position and direction consistently, you do.
28-30
: Parallel segment guard, wise it is.Prevents zero-division error, this check does. Strong your code has become.
32-34
: Scale logic, correct it seems.Well-handled is the thickness scaling, given the parallel check.
41-43
: Conditional drift check, good.Yes, only if perpendicular component and z non-zero, drift is.
45-46
: Opposite drift directions, confirm them you should.Entry drifted forward, exit backward, correct the effect may be.
Would you like to verify the sign convention is consistent across the rest of the code? If so, a script can I provide.
49-51
: Final return of segments, neat it is.Yes, both 2D and 3D segments returned in one place, clarity this provides.
Fatras/include/ActsFatras/Digitization/PlanarSurfaceDrift.hpp (8)
14-14
: Additional include, correct.Enables
Acts::Result
usage.
17-17
: Tuple header indeed required.Needed for returning combined segments, it is.
27-27
: 2D segment alias, well-labeled.Clear it is, to represent drifted pair in 2D.
29-29
: 3D segment alias, consistent naming.Yes, clarifies the original in local 3D, this does.
32-34
: Helpful doc line, yes.States the method’s purpose nicely, it does.
42-43
: Lorentz drift note, good clarity.Emphasizes 3D pixel scenario if perpendicular is zero, you do.
49-51
: Return info, well-explained.Yes, states plainly the content of the tuple, it does.
52-54
: Signature matches implementation, correct it is.Yes, consistent your function definition and doc are.
acbff52
to
44e1d9b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Fatras/include/ActsFatras/Digitization/PlanarSurfaceDrift.hpp
(2 hunks)Tests/UnitTests/Fatras/Digitization/PlanarSurfaceDriftTests.cpp
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Fatras/include/ActsFatras/Digitization/PlanarSurfaceDrift.hpp
🧰 Additional context used
🪛 Cppcheck (2.10-2)
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 (16)
- GitHub Check: merge-sentinel
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2404_clang19
- GitHub Check: CI Bridge / linux_ubuntu_2204
- 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: unused_files
- GitHub Check: linux_ubuntu
- GitHub Check: macos
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2404_clang19, 20, clang++-19)
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: build_debug
- GitHub Check: docs
🔇 Additional comments (11)
Tests/UnitTests/Fatras/Digitization/PlanarSurfaceDriftTests.cpp (11)
16-19
: Necessary includes for enhanced tests, added they have.For visualization and bounds creation, additional headers required are. The RectangleBounds, Enumerate, fstream, and string, support the new enhanced test case, they do.
Also applies to: 23-25
31-31
: Renamed and clarified the test case is, hmm.From
PlanarSurfaceDrift
toPlanarSurfaceDriftSimpleTests
, renamed the test case has been. More descriptive name, it is, as now two test cases exist. Better organization of tests, this promotes.Also applies to: 73-73
50-53
: Updated to unpack tuple return values, the code has been.From returning a single segment to a tuple of
[noDriftSegment, oSegment]
, changed the API has. Properly unpacks the return value now, this test does. Structured bindings, a modern C++ feature this is, mmm.
64-67
: Consistent with new API, this change is.Similar to previous test case, properly unpacks the tuple into
noDriftSegment1
anddScale1
, this does. Consistent with the updated API design, yes.
69-72
: Updated variable names in assertions, you have.Correctly updated the checks to use new variable names
noDriftSegment1
instead of original names. Consistent with the API changes, this is.
75-109
: Comprehensive enhanced test case, added you have.A more thorough test setup with thick sensor, this creates. Entry, readout, and exit surfaces with proper transforms and bounds, defined well they are. Good practice to test component thoroughly, this is, yes. Visualization capability, valuable for debugging it will be.
110-139
: Surface output and intersection testing, wisely implemented.Named surfaces and proper intersection testing, added you have. Files for visualization created they are, which helpful for complex geometry understanding, they will be. Robust test for failure conditions also added.
179-184
: Multiple drift scenarios, test wisely you do.Different drift vector scenarios defined properly. Normal drift and angled drift both positive and negative, cover important test cases they do. Complete testing, essential for correct behavior it is.
198-244
: Drift validation tests, thorough they are.For each drift scenario:
- Call to
toReadout
with proper parameters- Unpack returned tuple correctly
- Transform back to global coordinates
- Write segment visualization
- Validate entry and exit positions against expected values
Comprehensive test coverage, this provides. Proper validation of drifted positions, critical for correctness it is.
246-254
: Special case testing, important it is.Test the special case when particle direction normal to surface is, you do. Equal start and end points in local coordinates, expected this is. Thickness verification in global coordinates, correct it is. Edge cases, always test we must.
255-258
: Error case testing, wisely added.When particle moves along xy plane, error expected is. Verify that error returned correctly, you do. Both success and failure paths, tested they must be. Complete test coverage, this provides.
Tests/UnitTests/Fatras/Digitization/PlanarSurfaceDriftTests.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Tests/UnitTests/Fatras/Digitization/PlanarSurfaceDriftTests.cpp (1)
75-178
: Comprehensive test setup, impressive this is.Created entry, readout, and exit surfaces you have. Visualization outputs generated will be. For complex 3D geometry understanding, helpful this approach is. More comments to explain purpose of each section, beneficial they would be.
Add comments to explain purpose of readout grid and surface representation, clarity they will bring:
+ // Create test surfaces with proper transformation auto entryTransform = Acts::Transform3( Acts::Translation3(cPosition - 0.5 * thickness * localZ) * rotationMatrix); + // Generate visualization output for surfaces // Write out all the surfaces in separate files std::ofstream fsout; fsout.open("PlanarSurfaceDrift_" + surfaceNames[isf] + ".obj"); + // Create grid for visualizing readout plane // Readout grid output std::ofstream gout; gout.open("PlanarSurfaceDrift_readout_2D.obj");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Tests/UnitTests/Fatras/Digitization/PlanarSurfaceDriftTests.cpp
(2 hunks)
🧰 Additional context used
🪛 Cppcheck (2.10-2)
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 (16)
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2404_clang19
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: macos
- GitHub Check: linux_ubuntu
- GitHub Check: linux_ubuntu_extra (ubuntu2404_clang19, 20, clang++-19)
- GitHub Check: unused_files
- GitHub Check: missing_includes
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: build_debug
🔇 Additional comments (10)
Tests/UnitTests/Fatras/Digitization/PlanarSurfaceDriftTests.cpp (10)
16-19
: Wise additions to includes, I see.For rectangle bounds and enumeration functionality, these includes needed are. The Force flows better through organized code, yes!
23-25
: File operations support, now available is.For writing test outputs and visualization data, these includes necessary are. Hmm, for debugging and visual inspection, useful they will be.
31-31
: Renamed test case, more clarity it brings.From
PlanarSurfaceDrift
toPlanarSurfaceDriftSimpleTests
, a better name this is. When multiple test cases exist, identify their purpose we must.
50-53
: Return value unpacking, evolved the API has.From single value to tuple, the return type changed has. A tuple with segment and scaling factor, more information it provides.
64-72
: Consistent unpacking of tuple, good practice this is.For all method calls, same pattern followed you have. Clear and consistent code, easier to maintain it is.
179-196
: Helper function and drift scenarios, well organized they are.Multiple drift scenarios tested they will be. Lambda function for writing segments, code duplication it reduces. Strong with the Force, this approach is.
199-244
: Thorough testing of drift scenarios, essential this is.Each drift scenario validated it is. Entry and exit positions checked they are. From 3D to 2D transformation verified it is. Drift scaling factor used correctly it seems. The dark side of bugs, no chance they have.
246-254
: Special case testing, complete your test coverage it makes.When particle normal to surface travels, edge case this is. Same entry and exit positions in 2D expected are. Distance in 3D equal to thickness should be. Verified these properties are.
255-259
: Error case handling, prepared for the unexpected you are.When particle moves along XY plane, fail it should. Verified this error case is. Robust code, anticipates failure it does.
50-254
:✅ Verification successful
API change properly handled throughout, I observe.
From single segment to tuple returning, the API changed has. Consistently throughout test cases, unpacked the tuple is. But verify this change in other parts of codebase, you should.
🏁 Script executed:
#!/bin/bash # Find all calls to toReadout method in the codebase echo "Searching for all calls to toReadout method..." rg "toReadout\(" --type cpp # Find the implementation of PlanarSurfaceDrift::toReadout echo -e "\nChecking the implementation of PlanarSurfaceDrift::toReadout..." rg "PlanarSurfaceDrift::toReadout" --type cpp -A 10 # Look for other files that might be affected by this API change echo -e "\nChecking for other files using PlanarSurfaceDrift..." rg "PlanarSurfaceDrift" --type cpp | grep -v "PlanarSurfaceDriftTests.cpp"Length of output: 3777
API change, consistent it is.
Hmmm, young padawan, the transition from a single segment to a tuple returning both 2D and 3D segments has been verified across the codebase. Searches across the repository confirm that all usages—including in the tests and the Channelizer module—properly unpack and handle the new tuple returned by
toReadout
. Your updated API is integrated consistently. Trust in your changes, you may.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok with the changes. Could you make the PR title a bit more descriptive though?
|
Changes the API of the charge smearing such that it returns both the drifted segment in 2D and the original (re-calculated) 3D segment in the silicon.
This allows to scale the drifted paths pixel back to the original path length in order to apply threshold cuts appropriately.
Previously, the 3D path length (that was already calculated in the charge drifter) was re-estimated, but since the full incident angle was not available anymore, it led to slightly incorrect results.
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat
,fix
,refactor
,docs
,chore
andbuild
types.Summary by CodeRabbit
New Features
channelize
method to return multiple segment values with improved error handling.toReadout
method to return a tuple of drifted 2D and undrifted 3D segments, providing additional context.DriftError
enumerator to expand error handling capabilities.Bug Fixes
Tests
These updates enhance functionality, improve error handling, and ensure thorough testing of the revised features.