Skip to content

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

asalzburger
Copy link
Contributor

@asalzburger asalzburger commented Mar 31, 2025

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!

  • Use a conventional commits prefix: quick summary
    • We mostly use feat, fix, refactor, docs, chore and build types.
  • A milestone will be assigned by one of the maintainers

Summary by CodeRabbit

  • New Features

    • Enhanced the channelize method to return multiple segment values with improved error handling.
    • Updated the toReadout method to return a tuple of drifted 2D and undrifted 3D segments, providing additional context.
    • Introduced a new DriftError enumerator to expand error handling capabilities.
  • Bug Fixes

    • Improved error reporting for drift-related issues with specific messages.
  • Tests

    • Renamed and restructured test cases to accommodate new return types and introduced more comprehensive testing scenarios.
    • Updated hash values for specific test files to reflect content alterations.

These updates enhance functionality, improve error handling, and ensure thorough testing of the revised features.

Copy link

coderabbitai bot commented Mar 31, 2025

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Changed, the code has been. In the Channelizer::channelize method, the 2D path accumulation logic removed has been, and now a tuple containing a drifted segment and a drift scaling factor is retrieved from PlanarSurfaceDrift::toReadout. In PlanarSurfaceDrift, the return type has evolved from a single segment to a tuple, with internal drift scale computation adjusted. Tests, they now unpack the tuple accordingly. Streamlined, the methods are, reducing complexity in drift handling.

Changes

File(s) Summary of Changes
Fatras/include/ActsFatras/Digitization/Channelizer.hpp Modified channelize: now retrieves a tuple (driftedSegment, fullSegment) from toReadout and applies scaling, removing the old 2D path logic.
Fatras/include/ActsFatras/Digitization/PlanarSurfaceDrift.hpp
Fatras/src/Digitization/PlanarSurfaceDrift.cpp
Updated toReadout: return type changed from Segment2D to Acts::Result<std::tuple<Segment2D, Segment3D>>; internal drift scale calculation simplified, old drift application removed.
Tests/UnitTests/Fatras/Digitization/PlanarSurfaceDriftTests.cpp Adjusted tests to unpack the returned tuple into segment and scaling factor variables, renaming variables accordingly.
Fatras/include/ActsFatras/Digitization/DigitizationError.hpp
Fatras/src/Digitization/DigitizationError.cpp
Added DriftError to DigitizationError enum and updated error messaging to include "Drift error occurred."

Possibly related PRs

  • feat: component wise digital clustering #4142: The changes in the main PR, specifically the updates to the channelize method that involve retrieving values from the toReadout function, are related to the modifications in the toReadout method of the retrieved PR, which also involves changes to its return type and internal logic. Both PRs focus on enhancing the functionality of methods that deal with segment processing and error handling.

Suggested labels

automerge

Poem

Modified, our code has been, yes,
Drift and scale in harmony they progress.
The old paths, removed into the night,
Simplicity and clarity now shining bright.
May the coding force guide you, with insight!


🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the Component - Fatras Affects the Fatras module label Mar 31, 2025
Copy link

@coderabbitai coderabbitai bot left a 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.
Unpacked dScale 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

📥 Commits

Reviewing files that changed from the base of the PR and between fb2a207 and 3334a93.

📒 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.

@github-actions github-actions bot added this to the next milestone Mar 31, 2025
@asalzburger
Copy link
Contributor Author

asalzburger commented Apr 1, 2025

A few pictures (they are produced by the unit tests):

snapshot00

And another one:

snapshot01

Different drift scenarios:

snapshot02

Copy link

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 3334a93 and bf851d6.

📒 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 to PlanarSurfaceDriftSimpleTests 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.

@asalzburger asalzburger requested a review from benjaminhuth April 1, 2025 13:37
Copy link

github-actions bot commented Apr 1, 2025

📊: Physics performance monitoring for f4ab9d4

Full contents

physmon summary

@github-actions github-actions bot added Component - Examples Affects the Examples module Changes Performance labels Apr 1, 2025
@asalzburger asalzburger force-pushed the fix-fatras-digi-2d-to-3d branch from acbff52 to 44e1d9b Compare April 2, 2025 08:57
Copy link

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between fe41a0c and 44e1d9b.

📒 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 to PlanarSurfaceDriftSimpleTests, 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 and dScale1, 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:

  1. Call to toReadout with proper parameters
  2. Unpack returned tuple correctly
  3. Transform back to global coordinates
  4. Write segment visualization
  5. 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.

Copy link

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 44e1d9b and c0d9002.

📒 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 to PlanarSurfaceDriftSimpleTests, 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.

paulgessinger
paulgessinger previously approved these changes Apr 8, 2025
Copy link
Member

@paulgessinger paulgessinger left a 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes Performance Component - Examples Affects the Examples module Component - Fatras Affects the Fatras module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants