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: SonarCloud fixes, part 0 #3942

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

Conversation

asalzburger
Copy link
Contributor

@asalzburger asalzburger commented Dec 4, 2024

Some hopefully easy to fix SonarCloud issues closed.

--- END COMMIT MESSAGE ---

Summary by CodeRabbit

  • New Features

    • Enhanced type safety by adding the explicit keyword to multiple constructors across various classes, preventing implicit conversions.
    • Improved error handling and clarity in particle state management within simulation and data processing classes.
  • Bug Fixes

    • Refined method calls for accessing particle states, ensuring consistency in naming conventions (e.g., initial() to initialState()).
  • Documentation

    • Updated comments and logging for clarity regarding the handling of particle states and configuration parameters.
  • Refactor

    • Streamlined initialization syntax in unit tests for polygon bounds, enhancing readability and maintainability.

Copy link

coderabbitai bot commented Dec 4, 2024

Walkthrough

A modification made to various classes in the Acts library, enhancing type safety by adding the explicit keyword to multiple constructors. This change prevents implicit conversions for constructors accepting parameters like arrays and context objects. Additionally, method names related to particle states in the FatrasSimulation class and others have been updated for clarity, reflecting a shift from initial() and final() to initialState() and finalState(). Error handling and core functionalities remain intact throughout the changes.

Changes

File Path Change Summary
Alignment/include/ActsAlignment/Kernel/Alignment.ipp Updated calculateAlignmentParameters to cast alignResult.numTracks to double for floating-point division; added comments and logging.
Core/include/Acts/Definitions/Units.hpp Changed user literal operator functions from constexpr to consteval for compile-time evaluation.
Core/include/Acts/Geometry/TrapezoidVolumeBounds.hpp Added explicit keyword to constructor preventing implicit conversions from std::array<double, eSize>.
Core/include/Acts/MagneticField/ConstantBField.hpp Marked Cache constructor as explicit and updated makeCache to use std::in_place_type<Cache>.
Core/include/Acts/MagneticField/InterpolatedBFieldMap.hpp Marked Cache and InterpolatedBFieldMap constructors as explicit.
Core/include/Acts/MagneticField/NullBField.hpp Marked Cache constructor as explicit.
Core/include/Acts/MagneticField/SolenoidBField.hpp Marked Cache and SolenoidBField constructors as explicit.
Core/include/Acts/Material/AccumulatedSurfaceMaterial.hpp Marked constructor as explicit.
Core/include/Acts/Material/HomogeneousVolumeMaterial.hpp Marked constructor as explicit.
Core/include/Acts/Material/ISurfaceMaterial.hpp Marked constructor as explicit.
Core/include/Acts/Surfaces/AnnulusBounds.hpp Marked constructor as explicit.
Core/include/Acts/Surfaces/ConeBounds.hpp Marked constructor as explicit.
Core/include/Acts/Surfaces/ConvexPolygonBounds.hpp Marked multiple constructors as explicit.
Core/include/Acts/Surfaces/CylinderBounds.hpp Marked constructor as explicit.
Core/include/Acts/Surfaces/DiamondBounds.hpp Marked constructor as explicit.
Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp Marked constructor as explicit.
Core/include/Acts/Surfaces/EllipseBounds.hpp Marked constructor as explicit.
Core/include/Acts/Surfaces/LineBounds.hpp Marked constructor as explicit.
Core/include/Acts/Surfaces/RadialBounds.hpp Marked constructor as explicit.
Core/include/Acts/Surfaces/RectangleBounds.hpp Updated constructor to be explicit.
Core/include/Acts/Surfaces/SurfaceArray.hpp Marked constructors in SingleElementLookup and SurfaceArray as explicit.
Core/include/Acts/Utilities/Axis.hpp Marked iterator constructor in NeighborHoodIndices as explicit.
Core/include/Acts/Utilities/Grid.hpp Marked multiple constructors as explicit.
Core/include/Acts/Utilities/KDTree.hpp Marked constructor as explicit.
Core/include/Acts/Utilities/RangeXD.hpp Marked constructor as explicit.
Examples/Algorithms/Fatras/src/FatrasSimulation.cpp Renamed methods initial() to initialState() and final() to finalState(), refined error handling.
Examples/Algorithms/Geant4/src/ParticleTrackingAction.cpp Updated method to set final state from final() to finalState().
Examples/Algorithms/Generators/ActsExamples/Generators/EventGenerator.cpp Updated method calls to set particle positions to use initialState() and finalState().
Examples/Algorithms/HepMC/src/HepMCProcessExtractor.cpp Updated method calls to set final state from final() to finalState().
Examples/Framework/include/ActsExamples/EventData/GeometryContainers.hpp Renamed parameter in selectModule function for clarity.
Examples/Framework/include/ActsExamples/EventData/SimParticle.hpp Renamed member functions from initial() and final() to initialState() and finalState().
Examples/Io/EDM4hep/src/EDM4hepReader.cpp Updated method calls to use finalState() instead of final().
Examples/Io/EDM4hep/src/EDM4hepUtil.cpp Updated method calls to use initialState() and finalState() for particle properties.
Examples/Io/Root/src/RootAthenaDumpReader.cpp Enhanced error handling and updated method signatures for reading particles and measurements.
Examples/Io/Root/src/RootParticleReader.cpp Updated method calls to use initialState() and finalState().
Examples/Io/Root/src/SeedingPerformanceWriter.cpp Updated method calls to use initialState() in plot tools.
Examples/Io/Root/src/TrackFinderPerformanceWriter.cpp Updated method calls to use initialState() in plot tools.
Examples/Io/Root/src/TrackFitterPerformanceWriter.cpp Updated method calls to use initialState() in residual plots.
Tests/UnitTests/Core/Surfaces/ConvexPolygonBoundsTests.cpp Changed initialization syntax for poly<N> objects for clarity.

Possibly related PRs

Suggested labels

automerge

Suggested reviewers

  • andiwand
  • paulgessinger

"In code we trust, with changes so bright,
Explicit constructors, a clearer sight.
From initial to state, our methods now flow,
Enhancing our logic, watch our code grow!
Type safety we cherish, with joy we embrace,
For in every function, we find our true grace." 🌟


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 00cdf91 and 138cdb8.

📒 Files selected for processing (1)
  • Core/include/Acts/Definitions/Units.hpp (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • Core/include/Acts/Definitions/Units.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 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.

@asalzburger asalzburger added this to the next milestone Dec 4, 2024
@github-actions github-actions bot added Component - Core Affects the Core module Component - Examples Affects the Examples module Event Data Model labels Dec 4, 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

🧹 Outside diff range and nitpick comments (5)
Core/include/Acts/MagneticField/SolenoidBField.hpp (1)

74-74: Hmmmm, unused parameter, I sense!

Explicit constructor, good it is. But unused parameter /mctx/, remove we should. If context needed not, delete parameter we must.

-    explicit Cache(const MagneticFieldContext& /*mctx*/) {}
+    explicit Cache() {}
Core/include/Acts/Surfaces/CylinderBounds.hpp (1)

Line range hint 62-69: A pattern in the Force, I sense.

Across all bounds classes, a consistent pattern emerges. Type safety through explicit constructors, a wise architectural decision it is. Consider applying this pattern to future geometry classes, you should.

Also applies to: 60-60, 84-84

Core/include/Acts/Definitions/Units.hpp (1)

205-210: Wise change to consteval, this is!

At compile-time, unit conversions now happen, hmm. Prevent runtime overhead, this will. More efficient and safer, your code becomes.

Consider this pattern for other compile-time constants throughout the codebase, you should.

Alignment/include/ActsAlignment/Kernel/Alignment.ipp (1)

121-121: Prevent integer division pitfalls, this cast does!

More precise calculations, static_cast<double> ensures. Floating-point division, guaranteed it is.

Consider this alternative, you might:

-  alignResult.averageChi2ONdf = sumChi2ONdf / static_cast<double>(alignResult.numTracks);
+  alignResult.averageChi2ONdf = sumChi2ONdf / alignResult.numTracks;
+  static_assert(std::is_floating_point_v<decltype(sumChi2ONdf)>, "sumChi2ONdf must be floating point");

Compile-time guarantee, this provides. But acceptable, the current solution also is.

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

78-78: Wise addition of explicit keyword, this is!

Prevent accidental conversions from vector_t to KDTree, this change does. A step towards stronger type safety, it represents.

Consider similar protection for other single-argument constructors in the codebase, you should. A pattern of wisdom, this establishes.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between af6a7b7 and a6479d5.

📒 Files selected for processing (39)
  • Alignment/include/ActsAlignment/Kernel/Alignment.ipp (1 hunks)
  • Core/include/Acts/Definitions/Units.hpp (1 hunks)
  • Core/include/Acts/Geometry/TrapezoidVolumeBounds.hpp (1 hunks)
  • Core/include/Acts/MagneticField/ConstantBField.hpp (1 hunks)
  • Core/include/Acts/MagneticField/InterpolatedBFieldMap.hpp (2 hunks)
  • Core/include/Acts/MagneticField/NullBField.hpp (1 hunks)
  • Core/include/Acts/MagneticField/SolenoidBField.hpp (2 hunks)
  • Core/include/Acts/Material/AccumulatedSurfaceMaterial.hpp (1 hunks)
  • Core/include/Acts/Material/HomogeneousVolumeMaterial.hpp (1 hunks)
  • Core/include/Acts/Material/ISurfaceMaterial.hpp (1 hunks)
  • Core/include/Acts/Surfaces/AnnulusBounds.hpp (1 hunks)
  • Core/include/Acts/Surfaces/ConeBounds.hpp (1 hunks)
  • Core/include/Acts/Surfaces/ConvexPolygonBounds.hpp (1 hunks)
  • Core/include/Acts/Surfaces/CylinderBounds.hpp (1 hunks)
  • Core/include/Acts/Surfaces/DiamondBounds.hpp (1 hunks)
  • Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp (1 hunks)
  • Core/include/Acts/Surfaces/EllipseBounds.hpp (1 hunks)
  • Core/include/Acts/Surfaces/LineBounds.hpp (1 hunks)
  • Core/include/Acts/Surfaces/RadialBounds.hpp (1 hunks)
  • Core/include/Acts/Surfaces/RectangleBounds.hpp (1 hunks)
  • Core/include/Acts/Surfaces/SurfaceArray.hpp (2 hunks)
  • Core/include/Acts/Utilities/Axis.hpp (1 hunks)
  • Core/include/Acts/Utilities/Grid.hpp (1 hunks)
  • Core/include/Acts/Utilities/KDTree.hpp (1 hunks)
  • Core/include/Acts/Utilities/RangeXD.hpp (1 hunks)
  • Examples/Algorithms/Fatras/src/FatrasSimulation.cpp (2 hunks)
  • Examples/Algorithms/Geant4/src/ParticleTrackingAction.cpp (1 hunks)
  • Examples/Algorithms/Generators/ActsExamples/Generators/EventGenerator.cpp (1 hunks)
  • Examples/Algorithms/HepMC/src/HepMCProcessExtractor.cpp (1 hunks)
  • Examples/Framework/include/ActsExamples/EventData/GeometryContainers.hpp (1 hunks)
  • Examples/Framework/include/ActsExamples/EventData/SimParticle.hpp (1 hunks)
  • Examples/Io/EDM4hep/src/EDM4hepReader.cpp (1 hunks)
  • Examples/Io/EDM4hep/src/EDM4hepUtil.cpp (2 hunks)
  • Examples/Io/Root/src/RootAthenaDumpReader.cpp (1 hunks)
  • Examples/Io/Root/src/RootParticleReader.cpp (2 hunks)
  • Examples/Io/Root/src/SeedingPerformanceWriter.cpp (1 hunks)
  • Examples/Io/Root/src/TrackFinderPerformanceWriter.cpp (1 hunks)
  • Examples/Io/Root/src/TrackFitterPerformanceWriter.cpp (2 hunks)
  • Tests/UnitTests/Core/Surfaces/ConvexPolygonBoundsTests.cpp (1 hunks)
👮 Files not reviewed due to content moderation or server errors (3)
  • Examples/Algorithms/HepMC/src/HepMCProcessExtractor.cpp
  • Core/include/Acts/Surfaces/AnnulusBounds.hpp
  • Examples/Framework/include/ActsExamples/EventData/SimParticle.hpp
🧰 Additional context used
📓 Learnings (2)
Examples/Algorithms/Fatras/src/FatrasSimulation.cpp (1)
Learnt from: andiwand
PR: acts-project/acts#3804
File: Examples/Algorithms/Fatras/src/FatrasSimulation.cpp:301-312
Timestamp: 2024-11-12T11:01:29.093Z
Learning: In `Examples/Algorithms/Fatras/src/FatrasSimulation.cpp`, when the final state of a particle is missing, it's acceptable to set `particleSimulated.final()` to `particleInitial`.
Examples/Io/EDM4hep/src/EDM4hepReader.cpp (1)
Learnt from: andiwand
PR: acts-project/acts#3804
File: Examples/Algorithms/Fatras/src/FatrasSimulation.cpp:301-312
Timestamp: 2024-11-12T11:01:29.093Z
Learning: In `Examples/Algorithms/Fatras/src/FatrasSimulation.cpp`, when the final state of a particle is missing, it's acceptable to set `particleSimulated.final()` to `particleInitial`.
🔇 Additional comments (42)
Examples/Io/Root/src/TrackFitterPerformanceWriter.cpp (2)

151-152: Approve this change, I do! Consistent with the Force, it is.

From initial() to initialState(), the path leads. Clearer naming conventions, we now follow. Hmmmm.


185-186: In harmony with the previous change, this modification is.

The same wisdom applies here - from initial() to initialState() we move. Consistency throughout the codebase, we maintain.

Verify the consistency across the codebase, we must:

✅ Verification successful

Successful, the transition from initial() to initialState() is

Searched through the codebase, I have. No instances of the old initial() method remain, while initialState() consistently used across all relevant files, it is. In harmony with the Force, this change stands.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining occurrences of the old method name
rg "\.initial\(\)" --type cpp

# Search for the new method name to confirm standardization
rg "\.initialState\(\)" --type cpp

Length of output: 1491

Examples/Io/Root/src/RootAthenaDumpReader.cpp (2)

Line range hint 28-28: Enhance Type Safety by Updating Constructors, You Must

Adding explicit keyword to constructors, prevent unintended implicit conversions it does. Consistency across codebase, it ensures.


675-675: Update the Correct State, You Should

In reprocessParticles, updating newParticle.finalState().setNumberOfHits() you do. Verify that finalState() reflects the particle's true final state, ensure you must.

Run this script to confirm finalState() correctness:

✅ Verification successful

Let me analyze the implementation and usage of finalState() to ensure we're updating the correct state. This will help us understand if the update to numberOfHits is consistent with the particle's true final state.


Correct the Implementation Is, Young Padawan

Verified the usage of finalState() throughout the codebase, I have. Consistent with the particle tracking system's design, the update of numberOfHits is. In SimParticle class, properly managed the final state is, with various attributes like position, momentum, and hits being tracked. The Force is strong with this implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check usage of `finalState()` in the codebase.

# Test: Search for `finalState().setNumberOfHits` usage. Expect: Consistent updates.
rg -A 2 $'finalState\(\)\.setNumberOfHits'

Length of output: 296


Script:

#!/bin/bash
# Search for finalState() method definition and usage
ast-grep --pattern 'finalState()' 

# Search for the class containing finalState method
ast-grep --pattern 'class $_ {
  $$$
  finalState() {
    $$$
  }
  $$$
}'

# Search for other methods updating finalState
rg -A 3 'finalState\(\)\.' 

Length of output: 8432

Core/include/Acts/Material/HomogeneousVolumeMaterial.hpp (1)

28-28: Prevent Implicit Conversions, The explicit Keyword Does

Constructor of HomogeneousVolumeMaterial, now explicit it is. Implicit conversions, avoided they are. Type safety, improved it is.

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

22-22: Constructor Explicit, Now It Is

In NullBField::Cache, the constructor explicit you have made. Implicit conversions, prevented they are. Consistency with other magnetic field classes, maintained it is.

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

42-42: Explicit Constructor, Aid It Will

Constructor of LineBounds, explicit now it is. Implicit conversions, avoided they are. Code clarity, enhanced it is.

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

25-25: Use The explicit Keyword, Wise It Is

In ConstantBField::Cache, explicit constructor now you have. Implicit conversions, disallowed they are. Type safety, strengthened it is.

Tests/UnitTests/Core/Surfaces/ConvexPolygonBoundsTests.cpp (1)

45-45: Approve these changes, I do!

From brace initialization to direct constructor calls, moved we have. Wise changes these are, preventing implicit conversions they do. Maintain the same functionality while improving type safety, we shall.

Also applies to: 53-53, 59-59

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

66-66: Strong with type safety, this one is!

Added the 'explicit' keyword we have, preventing accidental conversions it does. A wise path to safer code, this is.

Examples/Algorithms/Generators/ActsExamples/Generators/EventGenerator.cpp (1)

88-89: More descriptive names, these methods now have!

From initial() and final() to initialState() and finalState(), evolved we have. Clearer intention these names show.

✅ Verification successful

Consistent with the Force, these changes are!

Through the codebase, searched I have. No remnants of the old method names initial() and final() found were. Instead, initialState() and finalState() consistently used they are, across all files. A complete transition to the new naming, achieved it has been.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining old method names that might need updating
rg -l "\.initial\(\)" --type cpp
rg -l "\.final\(\)" --type cpp

Length of output: 67


Script:

#!/bin/bash
# Search for any remaining old method names with surrounding context
rg "initial\(\)" --type cpp -C 2
rg "final\(\)" --type cpp -C 2

# Also search for the new method names to understand the extent of changes
rg "initialState\(\)" --type cpp -C 2
rg "finalState\(\)" --type cpp -C 2

Length of output: 26159

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

71-71: Wise addition of explicit keyword, this is!

Prevent accidental conversions, it does. Safe from the dark side of implicit type conversions, your code now is.

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

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

Explicit constructor for Config parameter, prevent accidents it will. Clear intentions, your code now shows.

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

78-78: Clear intentions, your constructor now has!

Like a well-trained Padawan, explicit keyword brings balance to the Force. Safe from accidental array conversions, your ConeBounds class now is.

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

62-69: Approve this change, I do! Wisdom in preventing implicit conversions, there is.

The Force strong with this change it is. Explicit constructor prevents accidental conversions from array to RectangleBounds, safer code it makes. Proper error checking maintained it has.

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

60-60: Consistent with the ways of the Force, this change is.

Like its rectangular sibling, explicit constructor wisdom it shares. Prevent dark side of implicit conversions, it does.

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

84-84: Complete the circle of type safety, this change does.

The trilogy of bounds classes, now complete it is. Explicit constructors they all have, bringing balance to the Force.

Core/include/Acts/Material/AccumulatedSurfaceMaterial.hpp (1)

40-40: Wise decision, adding explicit keyword is! Hmmmm.

Prevent accidental conversions from double to AccumulatedSurfaceMaterial, this change does. Good practice in modern C++, it is.

Core/include/Acts/Material/ISurfaceMaterial.hpp (1)

38-38: Strong with the Force, this change is! Yes, hmmm.

Explicit constructor, prevent implicit conversions it does. Consistency across the codebase, maintain it does.

Examples/Io/Root/src/RootParticleReader.cpp (2)

160-160: Clearer naming, better understanding brings! Hmmmm.

From initial() to initialState(), more descriptive the method name becomes. Consistent with the Force, this change is.


170-170: Balance to the Force, this change brings!

Like its companion initialState(), renamed finalState() brings harmony to the code. Symmetry in naming, important it is.

Examples/Algorithms/Geant4/src/ParticleTrackingAction.cpp (1)

98-98: Approve the method name change, I do!

Clearer naming convention this is, from final() to finalState(). Consistent with codebase changes, it remains. Approve this modification, I must!

Core/include/Acts/Geometry/TrapezoidVolumeBounds.hpp (1)

87-87: Wisdom in explicit constructor, there is!

Prevent accidental conversions, this change does. Safer code it creates, when constructing TrapezoidVolumeBounds from array values. Modern C++ practices, it follows!

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

91-101: Strong with type safety, these constructors become!

Consistent pattern of wisdom, I see. Three constructors, explicit they now are:

  • From vector of vertices
  • From fixed vertex array
  • From fixed value array

Prevent unexpected conversions, they do. Harmonious with modern C++ practices, this is!

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

62-62: Wise decision, making constructor explicit is!

Prevent accidental conversions from array to DiscTrapezoidBounds, this change does. Stronger type safety, it brings.

Examples/Io/Root/src/SeedingPerformanceWriter.cpp (1)

178-180: Verify the method name changes across the codebase, we must.

From initial() to initialState(), the change moves. Consistent with other files, this must be.

✅ Verification successful

The Force guides us to search with more patterns, it does. Both the old and new method names, we must find. Through AST and regex, clarity we shall achieve.


Correct, the changes are. Consistent throughout the codebase, they remain.

Through the Force, seen I have that initialState() is the only method used across all files. No instances of the old initial() method remain, hmm. The API change, complete it is. In harmony with the codebase, these modifications are.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining uses of initial() or final()
rg -A 2 '\.(initial|final)\(\)'

Length of output: 33


Script:

#!/bin/bash
# Search for any remaining uses of initial() or final() with more context
rg -A 2 '(->|\.)(initial|final)\(\)'

# Search for initialState usage to verify the new pattern
rg -A 2 'initialState\(\)'

# Use AST to find method calls more reliably
ast-grep --pattern '($_).initial()'
ast-grep --pattern '($_).final()'
ast-grep --pattern '($_).initialState()'

Length of output: 11928

Examples/Framework/include/ActsExamples/EventData/GeometryContainers.hpp (1)

165-169: Wise renaming, this is! From 'module' to 'imodule', clarity flows.

Avoid confusion with 'module' keyword, this change does. Better readability, it brings.

Examples/Io/EDM4hep/src/EDM4hepUtil.cpp (2)

38-48: Clear intentions, your code now shows!

From initial() to initialState(), the path leads. More explicit and clearer to understand, this naming becomes.


65-67: Consistent with the Force, these changes are!

To finalState() from final(), your journey continues. Harmony in naming conventions, this brings to the codebase.

Examples/Io/Root/src/TrackFinderPerformanceWriter.cpp (1)

263-270: In balance with other changes, this code is!

The path from initial() to initialState(), consistently followed you have. Through performance measurements, this harmony flows.

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

149-149: Wise decision, making the Cache constructor explicit is!

Prevent accidental conversions, this change does. Safer code, it creates.


178-179: Strong with the Force, this explicit constructor is!

Configuration parameters, protected from implicit conversions they are. Clear intent, the code now shows.

Examples/Algorithms/Fatras/src/FatrasSimulation.cpp (2)

225-225: Clearer naming, you have chosen!

From initial() to initialState(), more descriptive the method name becomes. Approve this change, I do.


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

Like its companion initialState(), renamed to finalState() this method is. Balance to the code, it brings.

Examples/Io/EDM4hep/src/EDM4hepReader.cpp (2)

283-286: Consistent logging changes, these are!

In harmony with the API changes, the debug logging statements now are. Well aligned with the new method name, they remain.


270-279: Hmmmm, approve these changes, I do!

Consistent with the API changes, these modifications are. From final() to finalState(), the method calls have evolved, yet their essence remains unchanged.

Verify the completeness of these changes throughout the codebase, we must:

✅ Verification successful

Consistent in the Force, these changes are!

Through the codebase, searched I have. All instances of finalState() properly used they are. No remnants of the old final() method remain, and uniformly applied the new API is. The changes in EDM4hepReader.cpp, aligned with the rest of the codebase they are.

In the following files, the new finalState() method correctly used it is:

  • Examples/Io/Root/src/RootParticleReader.cpp
  • Examples/Io/Root/src/RootAthenaDumpReader.cpp
  • Examples/Io/EDM4hep/src/EDM4hepUtil.cpp
  • Examples/Algorithms/HepMC/src/HepMCProcessExtractor.cpp
  • Examples/Algorithms/Geant4/src/ParticleTrackingAction.cpp
  • Examples/Algorithms/Fatras/src/FatrasSimulation.cpp
  • Examples/Algorithms/Generators/ActsExamples/Generators/EventGenerator.cpp

Complete and consistent, this API transition is. May the Force be with these changes!

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining instances of the old method name
rg "\.final\(\)" --type cpp

Length of output: 29


Script:

#!/bin/bash
# Search for both old and new method names to understand the context better
rg "\.(final|finalState)\(\)" --type cpp -A 2

# Also search for particle simulation related classes
ast-grep --pattern 'class $_ {
  $$$
  final() {
    $$$
  }
  $$$
}'

ast-grep --pattern 'class $_ {
  $$$
  finalState() {
    $$$
  }
  $$$
}'

Length of output: 3549

Core/include/Acts/Surfaces/SurfaceArray.hpp (3)

359-359: Strong with type safety, this constructor now is!

Prevent implicit conversions, the explicit keyword does. A wise choice for single-argument constructors, this is.


364-364: In balance with its sibling constructor, this one now is!

Consistency in type safety, this change maintains. Well done, this is.


445-445: Complete, the circle of type safety now is!

The final piece of the puzzle, this change represents. A consistent pattern of explicit constructors, we now have.

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

70-72: Approve the addition of explicit keyword, I do!

Prevent accidental conversions from std::pair to RangeXD, this change does. Safer code, it creates. Good practice for type safety, this is!

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

101-132: Wise changes to Grid constructors, these are!

Made explicit, all constructors now are. Prevent accidental conversions from tuples and parameter packs, this does. A systematic improvement in type safety, we have achieved.

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

46-46: Approve the explicit iterator constructor, I must!

Prevent implicit conversion from size_t to iterator, this change does. For iterators, particularly important this is, as subtle bugs from implicit conversions we must avoid.

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

🧹 Outside diff range and nitpick comments (1)
Examples/Framework/include/ActsExamples/EventData/SimParticle.hpp (1)

112-177: Documentation improvement suggest, I must!

Clear it is not, why some methods use initialState() while others use finalState(). Document this choice in class documentation, we should. Help future padawans understand the design, it will!

Add class-level documentation explaining:

  • Which properties are taken from initial state vs final state
  • Why this design choice was made
  • When to use each state in client code
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between a6479d5 and 00cdf91.

📒 Files selected for processing (14)
  • Alignment/include/ActsAlignment/Kernel/Alignment.ipp (1 hunks)
  • Core/include/Acts/Geometry/TrapezoidVolumeBounds.hpp (1 hunks)
  • Core/include/Acts/Surfaces/AnnulusBounds.hpp (1 hunks)
  • Core/include/Acts/Surfaces/ConvexPolygonBounds.hpp (1 hunks)
  • Core/include/Acts/Surfaces/CylinderBounds.hpp (1 hunks)
  • Core/include/Acts/Surfaces/DiamondBounds.hpp (1 hunks)
  • Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp (1 hunks)
  • Core/include/Acts/Surfaces/EllipseBounds.hpp (1 hunks)
  • Core/include/Acts/Surfaces/RectangleBounds.hpp (1 hunks)
  • Core/include/Acts/Surfaces/SurfaceArray.hpp (2 hunks)
  • Core/include/Acts/Utilities/Axis.hpp (1 hunks)
  • Core/include/Acts/Utilities/Grid.hpp (1 hunks)
  • Examples/Framework/include/ActsExamples/EventData/SimParticle.hpp (1 hunks)
  • Examples/Io/EDM4hep/src/EDM4hepUtil.cpp (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
  • Alignment/include/ActsAlignment/Kernel/Alignment.ipp
  • Core/include/Acts/Utilities/Axis.hpp
  • Core/include/Acts/Surfaces/DiamondBounds.hpp
  • Core/include/Acts/Surfaces/AnnulusBounds.hpp
  • Core/include/Acts/Surfaces/EllipseBounds.hpp
  • Core/include/Acts/Surfaces/CylinderBounds.hpp
  • Core/include/Acts/Surfaces/RectangleBounds.hpp
  • Core/include/Acts/Geometry/TrapezoidVolumeBounds.hpp
  • Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp
  • Core/include/Acts/Surfaces/ConvexPolygonBounds.hpp
  • Core/include/Acts/Surfaces/SurfaceArray.hpp
  • Core/include/Acts/Utilities/Grid.hpp
  • Examples/Io/EDM4hep/src/EDM4hepUtil.cpp
🔇 Additional comments (3)
Examples/Framework/include/ActsExamples/EventData/SimParticle.hpp (3)

59-68: Approve these changes, I do! Clearer names, they are!

Renamed methods better express their intent, they do. From initial()/final() to initialState()/finalState(), more explicit they become. Maintain const-correctness, they do, hmm.


82-107: Consistent changes throughout the setters, I sense!

Updated all setter methods to use new accessor names, they did. Maintain behavior while improving clarity, these changes do.


76-77: Verify all callers updated, we must!

Changed method names in withParticleId, correctly applied they are. But verify all callers throughout the codebase, we must!

✅ Verification successful

The previous search might have been too specific. Let me broaden the search to look for the new method names and the withParticleId usage to verify the changes are consistent across the codebase. This will help us see the context of these method calls and ensure all callers are properly updated.


Consistent, the changes are. Worry, we need not!

Through the Force, seen I have that all usages of initialState() and finalState() are properly aligned. The withParticleId method correctly uses these accessors in its implementation, and all callers throughout the codebase use the current method names. No remnants of old method names, there are.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining uses of old method names
rg -l "\.initial\(\)" --type cpp
rg -l "\.final\(\)" --type cpp

Length of output: 67


Script:

#!/bin/bash
# Search for any remaining uses of old method names with more context
rg "initialState\(\)" --type cpp -A 2
rg "finalState\(\)" --type cpp -A 2

# Also search for withParticleId to see all usages
rg "withParticleId" --type cpp -A 2

Length of output: 21985

Copy link

github-actions bot commented Dec 4, 2024

📊: Physics performance monitoring for 138cdb8

Full contents

physmon summary

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 Event Data Model
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant