Skip to content
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

chore: Make use of algebra shorthands #3998

Merged
merged 3 commits into from
Dec 18, 2024

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Dec 18, 2024

Summary by CodeRabbit

  • New Features

    • Enhanced type consistency across various classes and methods, particularly for matrix representations.
    • Added new test cases for the Gx2Fitter and ImpactPointEstimator to improve coverage and robustness.
  • Bug Fixes

    • Adjusted variable types in tests to align with updated matrix types, ensuring accurate calculations and validations.
  • Documentation

    • Updated comments and documentation to reflect changes in type definitions and method signatures.
  • Chores

    • Refactored code to replace ActsMatrix with SquareMatrix types for better clarity and maintainability across the codebase.

@andiwand andiwand added this to the next milestone Dec 18, 2024
Copy link

coderabbitai bot commented Dec 18, 2024

Walkthrough

Hmm, a transformation of types, we witness! Throughout the Acts library, a systematic replacement of ActsMatrix with more specialized SquareMatrix types, this pull request brings. Rotation matrices, magnetic field gradients, track parameters - all touched by this change, they are. A refinement of type definitions, not a revolution, but a subtle evolution of the codebase, this represents.

Changes

File Path Change Summary
Core/include/Acts/Definitions/Algebra.hpp Rotation matrix type definitions updated from ActsMatrix to SquareMatrix
Core/include/Acts/EventData/* Vector and matrix types streamlined across multiple event data classes
Core/include/Acts/MagneticField/* getFieldGradient method signatures updated with SquareMatrix3
Core/include/Acts/Surfaces/* Jacobian and rotation matrix types refined
Core/src/* Implementation files updated to match new type definitions
Tests/* Test cases adjusted to use new matrix types

Suggested labels

automerge

Suggested reviewers

  • paulgessinger
  • AJPfleger

Poem

Matrix types, they dance and sway
From ActsMatrix to SquareMatrix today
Refactoring code with Jedi grace 🌟
Types align in their new embrace
Cleaner, sharper - the Force's way! 🚀

Possibly related PRs

Hmm, a subtle change, this is. But important, it remains! Clarity and precision, the path to clean code they are. 🧘‍♂️🌈


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 07562cd and 028a211.

📒 Files selected for processing (4)
  • Core/include/Acts/MagneticField/ConstantBField.hpp (1 hunks)
  • Core/include/Acts/MagneticField/MultiRangeBField.hpp (1 hunks)
  • Core/include/Acts/MagneticField/NullBField.hpp (1 hunks)
  • Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp
  • Core/include/Acts/MagneticField/NullBField.hpp
  • Core/include/Acts/MagneticField/ConstantBField.hpp
  • Core/include/Acts/MagneticField/MultiRangeBField.hpp

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?

❤️ Share
🪧 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. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @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 Component - Core Affects the Core module Component - Examples Affects the Examples module Component - Plugins Affects one or more Plugins Event Data Model Vertexing Track Fitting labels Dec 18, 2024
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 (5)
Plugins/DD4hep/src/DD4hepFieldAdapter.cpp (1)

54-54: Implement, we must, the gradient once decided.

Currently returns NotImplemented, yes. Provide a partial or full gradient in the future, perhaps.

Would you like help to open an issue for implementing the gradient retrieval?

Core/include/Acts/MagneticField/MultiRangeBField.hpp (1)

64-65: Approve this change, I do. Yet incomplete implementation remains, hmm.

Changed from ActsMatrix<3, 3> to SquareMatrix3, the parameter type has been. Consistent with the codebase's evolution, this is. But implement this method fully, we must.

Help implement the field gradient calculation, shall I? Open an issue to track this task, we can.

Core/include/Acts/MagneticField/ConstantBField.hpp (1)

54-55: Approve this change, I do. Yet calculate the derivative, we must.

Changed the type we have, maintaining harmony with other field providers. But incomplete our task remains - calculate the derivative, we do not. For a constant field, zero the gradient should be, yes.

A simple implementation, suggest I do:

   Result<Vector3> getFieldGradient(
       const Vector3& position, SquareMatrix3& derivative,
       MagneticFieldProvider::Cache& cache) const override {
     (void)position;
-    (void)derivative;
+    derivative.setZero();  // Gradient of constant field is zero
     (void)cache;
     return Result<Vector3>::success(m_BField);
   }
Core/include/Acts/Definitions/Algebra.hpp (1)

80-81: Strong with the Force, these changes are!

From ActsMatrix to SquareMatrix, the transition flows naturally. For rotation matrices, square they must always be! Wisdom in this change lies - clearer intent it shows, better understanding it brings.

Consider documenting in comments that rotation matrices must maintain orthogonality property, young padawan. Help future maintainers, this would.

Core/src/Vertexing/AdaptiveMultiVertexFitter.cpp (1)

47-48: A path to enlightenment through type harmony, I sense!

Throughout the codebase, a systematic transformation from generic matrix types (ActsMatrix, ActsVector) to more specific algebra types (SquareMatrix2, Vector2) unfolds. Benefits, many there are:

  • Clearer intent through specific types
  • Enhanced type safety through specialized matrices
  • Greater consistency across the Force... err, codebase

Also applies to: 85-87, 281-281

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9269562 and bafedb1.

📒 Files selected for processing (36)
  • Core/include/Acts/Definitions/Algebra.hpp (1 hunks)
  • Core/include/Acts/EventData/GenericBoundTrackParameters.hpp (1 hunks)
  • Core/include/Acts/EventData/MultiTrajectoryBackendConcept.hpp (3 hunks)
  • Core/include/Acts/EventData/TrackStateProxyConcept.hpp (1 hunks)
  • Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp (1 hunks)
  • Core/include/Acts/EventData/detail/TestSourceLink.hpp (2 hunks)
  • Core/include/Acts/MagneticField/ConstantBField.hpp (1 hunks)
  • Core/include/Acts/MagneticField/InterpolatedBFieldMap.hpp (1 hunks)
  • Core/include/Acts/MagneticField/MagneticFieldProvider.hpp (1 hunks)
  • Core/include/Acts/MagneticField/MultiRangeBField.hpp (1 hunks)
  • Core/include/Acts/MagneticField/NullBField.hpp (1 hunks)
  • Core/include/Acts/MagneticField/SolenoidBField.hpp (1 hunks)
  • Core/include/Acts/Propagator/EigenStepperDefaultExtension.hpp (1 hunks)
  • Core/include/Acts/Propagator/EigenStepperDenseExtension.hpp (1 hunks)
  • Core/include/Acts/Surfaces/CylinderBounds.hpp (1 hunks)
  • Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp (1 hunks)
  • Core/include/Acts/Utilities/VectorHelpers.hpp (1 hunks)
  • Core/src/MagneticField/MultiRangeBField.cpp (1 hunks)
  • Core/src/MagneticField/SolenoidBField.cpp (1 hunks)
  • Core/src/Surfaces/AnnulusBounds.cpp (1 hunks)
  • Core/src/Surfaces/CylinderBounds.cpp (1 hunks)
  • Core/src/Surfaces/DiscTrapezoidBounds.cpp (1 hunks)
  • Core/src/Utilities/SpacePointUtility.cpp (2 hunks)
  • Core/src/Vertexing/AdaptiveGridTrackDensity.cpp (1 hunks)
  • Core/src/Vertexing/AdaptiveMultiVertexFitter.cpp (1 hunks)
  • Examples/Detectors/MagneticField/include/ActsExamples/MagneticField/ScalableBField.hpp (1 hunks)
  • Examples/Io/Root/src/VertexNTupleWriter.cpp (2 hunks)
  • Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepFieldAdapter.hpp (1 hunks)
  • Plugins/DD4hep/src/DD4hepFieldAdapter.cpp (1 hunks)
  • Tests/Benchmarks/AnnulusBoundsBenchmark.cpp (1 hunks)
  • Tests/UnitTests/Core/Geometry/VolumeTests.cpp (1 hunks)
  • Tests/UnitTests/Core/MagneticField/InterpolatedBFieldMapTests.cpp (1 hunks)
  • Tests/UnitTests/Core/Propagator/BoundToCurvilinearConversionTests.cpp (3 hunks)
  • Tests/UnitTests/Core/TrackFitting/Gx2fTests.cpp (1 hunks)
  • Tests/UnitTests/Core/Utilities/HelpersTests.cpp (2 hunks)
  • Tests/UnitTests/Core/Vertexing/ImpactPointEstimatorTests.cpp (1 hunks)
👮 Files not reviewed due to content moderation or server errors (1)
  • Examples/Io/Root/src/VertexNTupleWriter.cpp
🔇 Additional comments (35)
Tests/UnitTests/Core/TrackFitting/Gx2fTests.cpp (1)

998-998: Call out this offset, we shall.

A large shift of 100 mm per surface, introduced here is. Confirm that this behavior, intended it is for testing. In production, might cause confusion or unrealistic scenarios.

Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepFieldAdapter.hpp (1)

34-34: Much clarity in your new matrix type, I sense.

Replacing ActsMatrix<3, 3> with SquareMatrix3, consistent with the codebase it is. Simplify it shall the gradient interface.

Core/include/Acts/MagneticField/MagneticFieldProvider.hpp (1)

53-53: Uniform the matrix type is, aligned across the codebase, it becomes.

Good synergy with SquareMatrix3 throughout the field provider, yes.

Core/include/Acts/MagneticField/NullBField.hpp (1)

46-47: Correct this change is, hmm, yes.

For a null field, zero gradient there always is. Changed the type safely we have, as unused the parameter remains. Maintain consistency with other field providers, this does.

Core/src/MagneticField/MultiRangeBField.cpp (1)

74-74: Hmmmm, good this change is!

Changed from ActsMatrix<3, 3>& to SquareMatrix3&, the parameter type has been. Consistent with the algebra shorthand initiative, this is. Maintain the same functionality while improving type clarity, it does.

Tests/UnitTests/Core/Geometry/VolumeTests.cpp (1)

34-34: Balanced and harmonious, this change is!

From ActsMatrix<3, 3> to SquareMatrix3, the test adapts. Maintain its purpose while embracing the new way, it does.

Verify we must, that all rotation matrix tests maintain orthogonality:

✅ Verification successful

Let me examine the actual usage patterns to ensure all rotation matrix declarations are consistent, hmm.


Harmonious, the change is. No disturbance in the Force, I sense!

Through the Force, seen I have that all rotation matrices in the tests follow the new way. SquareMatrix3 and RotationMatrix3 used consistently they are, with no lingering ActsMatrix<3,3> for rotations remaining. The change in VolumeTests.cpp, aligned with the codebase's path, it is.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other rotation matrix tests that might need similar updates
rg -l "RotationMatrix[23]|ActsMatrix.*3.*3.*rotation" "Tests/"

Length of output: 1282


Script:

#!/bin/bash
# Check for any instances where ActsMatrix<3,3> is used for rotation matrices
rg "ActsMatrix.*3.*3.*rotation" Tests/ -B 2 -A 2

# Also check for RotationMatrix usage patterns
rg "RotationMatrix[23]" Tests/ -B 2 -A 2

Length of output: 17854

Core/src/Surfaces/DiscTrapezoidBounds.cpp (1)

42-44: Approve this change, I do!

Wisdom in using specialized types, there is. From generic ActsMatrix<2, 2> to specific SquareMatrix2, a path to clearer code, this change leads. Unchanged in its ways, the function's logic remains.

Examples/Detectors/MagneticField/include/ActsExamples/MagneticField/ScalableBField.hpp (1)

75-75: Harmonious with the Force, this change is!

Throughout the magnetic field providers, consistency we seek. From ActsMatrix<3, 3> to SquareMatrix3, a unified path we follow. Strong with the type system, your code becomes.

Core/include/Acts/EventData/MultiTrajectoryBackendConcept.hpp (1)

59-59: Balance to the type system, these changes bring!

Through the Force, I sense a pattern. From generic ActsVector to specific Vector types, your path leads. In const and mutable forms alike, consistency maintained is. Through multiple trajectory concepts, this wisdom flows.

Hmm, verify the impact across the codebase, we should.

Also applies to: 89-89, 110-110

Core/include/Acts/MagneticField/SolenoidBField.hpp (1)

119-119: Approve the type refinement, hmm, yes.

A wise change this is, using SquareMatrix3 instead of ActsMatrix<3, 3>. Consistent with the magnetic field provider family, it makes our code more specialized and clear, it does.

✅ Verification successful

Consistent in the Force, this change is, hmmmm.

Through the Force, I see that all magnetic field providers now use SquareMatrix3 in their getFieldGradient signatures. From the base MagneticFieldProvider to its disciples - SolenoidBField, ConstantBField, MultiRangeBField, NullBField, and even the DD4hepFieldAdapter - unified in their ways, they are. No remnants of the old ActsMatrix<3,3> lingering in the gradient calculations, there are.

The path to enlightenment through type refinement, clear it is. Strong with the Force, this change is.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify consistent usage of SquareMatrix3 across magnetic field providers
ast-grep --pattern 'getFieldGradient($$$, SquareMatrix3& $_, $$$)'

Length of output: 68


Script:

#!/bin/bash
# Search for getFieldGradient method signatures across magnetic field providers
rg -A 2 "getFieldGradient.*\(" --type cpp

# Also check for ActsMatrix usage in gradient calculations
rg "ActsMatrix.*3.*3.*derivative" --type cpp

# Find all magnetic field provider header files to analyze their interfaces
fd ".*Field.*\.hpp$" Core/include

Length of output: 6283

Tests/UnitTests/Core/MagneticField/InterpolatedBFieldMapTests.cpp (1)

120-120: Harmonious with implementation changes, this test modification is.

The Force is strong with this one. Changed from ActsMatrix<3, 3> to SquareMatrix3, the test variable has been. Balance to the codebase, this brings.

Core/include/Acts/EventData/detail/TestSourceLink.hpp (1)

44-44: A trilogy of wise changes, observe we must.

Changed the types have been, from verbose ActsVector<2> to concise Vector2. Three places touched:

  • The parameters member
  • The covariance initialization
  • The constructor parameter

Simplified yet powerful, these changes remain. The way of the Jedi, this is.

Also applies to: 55-55, 58-58

✅ Verification successful

Verified the changes are, consistent they remain!

The Force reveals three harmonious uses of Vector2, it does:

  • Member variable declaration: Acts::Vector2 parameters
  • Constructor initialization: Acts::Vector2(var, 0)
  • Parameter type: const Acts::Vector2& params

No traces of old ActsVector<2> remain, complete the transformation is. Balance to the code, these changes bring.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify consistent Vector2 usage in test source links
ast-grep --pattern 'TestSourceLink {
  $$$
  Vector2 $_
  $$$
}'

Length of output: 66


Script:

#!/bin/bash
# Search for TestSourceLink class definition and its usage of Vector2
rg -A 10 "class TestSourceLink" Core/include/Acts/EventData/detail/TestSourceLink.hpp

# Check for any other occurrences of ActsVector<2> that might need updating
rg "ActsVector<2>" Core/include/Acts/EventData/detail/TestSourceLink.hpp

# Look for Vector2 usage in the same file
rg "Vector2" Core/include/Acts/EventData/detail/TestSourceLink.hpp

Length of output: 361

Core/include/Acts/Surfaces/CylinderBounds.hpp (1)

148-148: Approve this change, I do!

A wise choice it is, to use SquareMatrix2 instead of ActsMatrix<2, 2>. Clearer intent and better readability, this brings to our code. Hmmmm.

Core/src/MagneticField/SolenoidBField.cpp (1)

61-61: Approve this change too, I must!

From ActsMatrix<3, 3>& to SquareMatrix3&, a natural evolution this is. Maintains the reference parameter's efficiency, it does. Hmmmm, yes.

Core/src/Surfaces/CylinderBounds.cpp (1)

Line range hint 38-44: Correct implementation, this is!

The Force is strong with this one! Properly initialized and assigned, the SquareMatrix2 elements are. Changed only in type, not in mathematical essence, the Jacobian remains. Hmmmm.

Tests/Benchmarks/AnnulusBoundsBenchmark.cpp (1)

55-55: Approve this change, I do!

A wise choice it is, to use SquareMatrix2 instead of ActsMatrix<2, 2>. More specific and clearer intent, it shows. Hmmmm.

Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp (1)

153-153: Wise decision, this return type change is!

From ActsMatrix<2, 2> to SquareMatrix2, the path leads. Clearer intent and better type safety, it brings. Hmmmm.

Core/include/Acts/Utilities/VectorHelpers.hpp (1)

181-182: Strong with the Force, this change is!

To SquareMatrix3 from ActsMatrix<3, 3>, the transformation flows naturally. More specific types, better code readability brings. Hmmmm.

Core/include/Acts/EventData/TrackStateProxyConcept.hpp (1)

31-34: Approve the type changes, I do! Hmmmm.

Wise decision it is, to use specialized Vector2 type instead of ActsVector<2>. Clearer intent and better readability, this brings.

✅ Verification successful

Safe, this change is! Verified, the type alias has been.

In Core/include/Acts/Definitions/Algebra.hpp, defined Vector2 as ActsVector<2> already is. A simple type alias change this represents, hmmmm. No impact on functionality there is, as the same underlying type remains. Through the Force, I sense that all usage of Vector2 in the codebase properly aligned with this definition is.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining ActsVector<2> usage that might need updating
rg "ActsVector<2>" --type cpp

Length of output: 104


Script:

#!/bin/bash
# Check if Vector2 is used consistently in measurement-related code
rg "Vector2.*Measurement" --type cpp -A 2 -B 2

# Check for any other measurement-related type definitions
ast-grep --pattern 'using $_ = $_<2>;'

Length of output: 89


Script:

#!/bin/bash
# Look for any direct usage of ActsVector<2> in measurement contexts
rg "Measurement.*ActsVector" --type cpp

Length of output: 347


Script:

#!/bin/bash
# Check the definition and usage context of Vector2
rg "Vector2" --type cpp -A 2 -B 2

Length of output: 75884

Core/include/Acts/Propagator/EigenStepperDefaultExtension.hpp (1)

178-181: Approve these changes, I must. But caution, we need!

From ActsMatrix<3,3> to SquareMatrix3, the transition is. Maintain numerical precision in transport calculations, we must. Wise to verify gradient calculations in tests, it would be.

✅ Verification successful

Safe, these changes are. Verified through the Force, they have been.

Through extensive test coverage in JacobianTests.cpp, see I do that transport matrices and transformations thoroughly tested are. The EigenStepper's numerical precision, maintained it shall be with these specialized types. The Force shows me that:

  • Jacobian and transport calculations, extensively tested they are across multiple surface types
  • Matrix operations in EigenStepperTests.cpp, validated they are
  • Type consistency throughout the stepper implementation, maintained it is
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for test coverage of transport matrix calculations
rg -A 5 "transportMatrix.*test" --type cpp

Length of output: 44


Script:

#!/bin/bash
# Search for test files related to transport calculations and gradients
rg -l "transport.*matrix|jacobian|gradient" Tests/

Length of output: 950


Script:

#!/bin/bash
# Search for test files specifically mentioning EigenStepper
rg -l "EigenStepper.*Test" Tests/

Length of output: 82


Script:

#!/bin/bash
# Look for numerical test assertions in stepper tests
rg "CHECK|BOOST_CHECK|EXPECT" Tests/ -A 3 -B 3 | rg -A 3 -B 3 "(transport|gradient|jacobian).*matrix"

Length of output: 103


Script:

#!/bin/bash
# Find usage of SquareMatrix3 in test files
ast-grep --pattern 'SquareMatrix3 $_ = $_'

Length of output: 44


Script:

#!/bin/bash
# Look into EigenStepperTests.cpp content
cat Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp

Length of output: 44901


Script:

#!/bin/bash
# Check for transport matrix tests with more specific pattern
rg "TEST.*transport|transport.*matrix" Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp -A 5

Length of output: 99


Script:

#!/bin/bash
# Look for gradient-related tests in EigenStepper
rg "gradient|jacobian" Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp -A 5

Length of output: 331


Script:

#!/bin/bash
# Look for tests of the EigenStepperDefaultExtension
rg -l "EigenStepperDefaultExtension" Tests/

Length of output: 151


Script:

#!/bin/bash
# Search for transport matrix initialization in the extension
ast-grep --pattern 'dk$_dT = SquareMatrix3::$_'

Length of output: 49


Script:

#!/bin/bash
# Look for numerical test assertions in JacobianTests
cat Tests/UnitTests/Core/Propagator/JacobianTests.cpp

Length of output: 9947

Tests/UnitTests/Core/Utilities/HelpersTests.cpp (3)

80-84: Approve the cross product test changes, I do!

Changed to SquareMatrix3, the test matrices have. Preserved, the test logic remains.


92-92: Approve the toString test changes, I do!

Consistent with other changes, this is. Maintained, the test coverage remains.


175-176: Hmmmm, template test types, changed they are!

Wise to keep testing various matrix sizes, it is. Both SquareMatrix3 and SquareMatrix4, covered they are.

✅ Verification successful

The previous ast-grep pattern might not have worked due to syntax complexity. Let's try with ripgrep and direct file inspection to verify the matrix testing coverage, hmm yes.


Correct, the test types are, hmmmm. Safe, the changes are!

In the test file, see I do that both SquareMatrix3 and SquareMatrix4 are properly tested in matrix multiplication tests. Part of a larger tuple of test types, they are:

using MatrixProductTypes =
    std::tuple<std::pair<SquareMatrix3, SquareMatrix3>,
               std::pair<SquareMatrix4, SquareMatrix4>,
               std::pair<ActsMatrix<8, 8>, ActsMatrix<8, 8>>,
               std::pair<ActsMatrix<8, 7>, ActsMatrix<7, 4>>>;

Comprehensive test coverage, there is. Both matrix sizes, tested they are with random values and blocked multiplication. Strong with the Force, this test implementation is!

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify that we have test coverage for all matrix types
ast-grep --pattern 'BOOST_AUTO_TEST_CASE.*Matrix.*\{'

Length of output: 55


Script:

#!/bin/bash
# Search for matrix-related test cases with ripgrep for better pattern matching
rg "BOOST_AUTO_TEST_CASE.*Matrix.*\{" -A 5

# Also look for matrix type definitions and usages
rg "SquareMatrix[34]" -A 2

# Check the specific test file content
cat Tests/UnitTests/Core/Utilities/HelpersTests.cpp

Length of output: 41914

Core/include/Acts/MagneticField/InterpolatedBFieldMap.hpp (1)

317-317: Hmmmm, approve this change, I do!

A wise choice it is, to use SquareMatrix3 instead of ActsMatrix<3, 3>. More specialized and clearer intent, it shows. Consistent with the algebra shorthands initiative, this change is.

Core/include/Acts/EventData/GenericBoundTrackParameters.hpp (2)

126-126: Approve this change, I must!

From ActsVector<2> to Vector2, a path to clarity this is. Simpler to read and understand, it becomes.


128-132: Wise implementation, this is!

Clean and elegant, the new implementation with Vector3 is. A good balance between readability and functionality, it achieves.

Core/src/Vertexing/AdaptiveGridTrackDensity.cpp (1)

184-184: Approve this change too, I shall!

To Vector3 from ActsVector<3>, a natural progression it is. With the rest of the codebase, harmony it brings.

Core/src/Utilities/SpacePointUtility.cpp (1)

85-87: Hmm, wise changes these are, young padawan!

Replaced ActsMatrix with more specific algebra types, you have. Maintain mathematical correctness while improving code clarity, these changes do.

✅ Verification successful

Consistent with the Force, these changes are!

Observe I do that jacXyzToRhoZ remains as ActsMatrix<2, 3>, while only the result jac transforms into SquareMatrix2. Mathematical harmony maintained this keeps, as the intermediate calculation requires the 2x3 form, hmm! The final 2x2 result, properly typed with SquareMatrix2 it now is.

Clear the path forward is:

  • ActsMatrix<2, 3> for jacXyzToRhoZ - correct it remains, for dimensional truth it speaks
  • SquareMatrix2 for jac - wise change this is, for square matrix of size 2 it truly represents
  • Vector2 for var - precise type this brings, matching the dimensional reality
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining ActsMatrix usage in coordinate transformations
ast-grep --pattern 'ActsMatrix<2, 2> $var = jacXyzToRhoZ'

Length of output: 59


Script:

#!/bin/bash
# Search for any remaining ActsMatrix usage in the file
rg "ActsMatrix" "Core/src/Utilities/SpacePointUtility.cpp" -B 2 -A 2

# Search for similar matrix operations in the codebase
ast-grep --pattern 'SquareMatrix2 $var = $_ * $_'

# Check for other usages of jacXyzToRhoZ
rg "jacXyzToRhoZ" -A 2 -B 2

Length of output: 2310

Core/src/Surfaces/AnnulusBounds.cpp (1)

281-281: Strong with the Force, this change is!

From ActsMatrix to SquareMatrix2, the Jacobian type has evolved. Preserve the mathematical essence while bringing harmony to the type system, it does.

Core/src/Vertexing/AdaptiveMultiVertexFitter.cpp (1)

47-48: Balanced and precise, this transformation is!

From ActsVector to Vector2, the position difference type has shifted. Maintain the vertex fitting wisdom while bringing unity to the type system, it does.

Core/include/Acts/Propagator/EigenStepperDenseExtension.hpp (1)

266-269: Approve these changes, I do! Hmmmm.

Wise choice it is, to use specialized SquareMatrix3 type for rotation matrices dk1dT through dk4dT. Clearer intent this brings, while maintaining same mathematical operations. Changed only the type declarations are, preserving the logic intact remains.

Tests/UnitTests/Core/Vertexing/ImpactPointEstimatorTests.cpp (1)

308-308: Consistent with the Force, this change is!

From ActsVector<4> to Vector4 we move, maintaining harmony with the codebase's evolution. A simple yet meaningful change this is, preserving all test functionality.

Tests/UnitTests/Core/Propagator/BoundToCurvilinearConversionTests.cpp (2)

140-140: Constructor parameter, updated wisely it is!

From ActsMatrix<3,3> to SquareMatrix3 the parameter moves, maintaining consistency with the new type system while preserving the test's essence.


151-151: Member variable type, changed harmoniously it is!

The surface_rot member embraces SquareMatrix3, it does. In perfect alignment with the constructor change this stands, preserving the test data structure's integrity.

Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp (1)

1238-1238: Approve the vector type change, I do!

Changed ActsVector<3> to Vector3, this test has. A more concise and specialized type for 3D vectors, it now uses. Consistent with the algebra shorthand initiative and improves readability, this change does.

Copy link

github-actions bot commented Dec 18, 2024

📊: Physics performance monitoring for 028a211

Full contents

physmon summary

@kodiakhq kodiakhq bot merged commit 32d4bc1 into acts-project:main Dec 18, 2024
42 checks passed
@andiwand andiwand deleted the chore-use-algebra-shothands branch December 18, 2024 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Component - Examples Affects the Examples module Component - Plugins Affects one or more Plugins Event Data Model Track Fitting Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants