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

refactor: Replace ParticleSmearing with TrackParameterSmearing in Examples #3784

Merged
merged 28 commits into from
Nov 29, 2024

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Oct 25, 2024

  • extract track parameters from particles
  • replace ParticleSmearing with TrackParameterSmearing

Summary by CodeRabbit

  • New Features

    • Introduced TrackParameterSmearing and ParticleTrackParamExtractor classes for enhanced track parameter processing.
    • Updated smearing configurations and parameters to improve clarity and usability.
    • Enhanced Python bindings for various generator classes, allowing for customizable event generation.
  • Bug Fixes

    • Adjusted logging levels for new algorithms to ensure appropriate verbosity.
  • Documentation

    • Updated comments and documentation to reflect changes in parameter names and functionalities.
  • Chores

    • Removed deprecated ParticleSmearing algorithm and associated files from the project.

@andiwand andiwand added this to the next milestone Oct 25, 2024
@github-actions github-actions bot added the Component - Examples Affects the Examples module label Oct 25, 2024
@andiwand andiwand changed the title refactor: Replace particle smearing by track parameter smearing in Examples refactor: Replace ParticleSmearing with TrackParameterSmearing in Examples Oct 25, 2024
Copy link

github-actions bot commented Oct 25, 2024

📊: Physics performance monitoring for 4f5daa0

Full contents

physmon summary

@github-actions github-actions bot added the Infrastructure Changes to build tools, continous integration, ... label Oct 28, 2024
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.

So the idea is that we just produce full in track parameters and update the smearing to run in track parameters?

@andiwand
Copy link
Contributor Author

andiwand commented Nov 2, 2024

I think smearing track parameters is more general than smearing particles to tracks. I enabled the generator to distribute true track parameters which can also be used for other stuff than smearing. I saw a couple of times that we disable the smearing with 0 sigmas which can now be purely handled by the generator.

@andiwand
Copy link
Contributor Author

andiwand commented Nov 2, 2024

Actually to make this work correctly I have to extract the track parameters from particles in a different spot. Right now the track parameters and truth proto tracks do not reliably align anymore. To solve it it makes more sense to have a separate extraction algorithm I believe.

Copy link

coderabbitai bot commented Nov 6, 2024

## Walkthrough
Focus on the changes, we must. Renaming and restructuring parameters related to particle tracking configurations, these changes do. The `ParticleSmearingSigmas` class, now `TrackSmearingSigmas`, renamed it is. Parameters updated, they are, from `d0`, `d0PtA`, and others, to `loc0`, `loc0PtA`, and more. New classes for extracting and smearing track parameters introduced, while the `ParticleSmearing` class removed it is. Clarity and consistency in the codebase enhanced, yes.

## Changes

| File | Change Summary |
|------|----------------|
| `CI/physmon/workflows/physmon_trackfinding_1muon.py` | Renamed `ParticleSmearingSigmas` to `TrackSmearingSigmas` and updated parameters from `d0`, `d0PtA`, etc., to `loc0`, `loc0PtA`, etc. |
| `Examples/Algorithms/Generators/ActsExamples/Generators/EventGenerator.cpp` | Removed namespace prefix `ActsExamples::` from class and method signatures, updated include directives. |
| `Examples/Algorithms/Generators/ActsExamples/Generators/EventGenerator.hpp` | Removed unused header `<functional>`, updated comment for `outputVertices` in `Config` struct. |
| `Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSmearing.cpp` | Deleted file, removed `ParticleSmearing` class and its methods. |
| `Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleTrackParamExtractor.cpp` | Added `ParticleTrackParamExtractor` class with methods for extracting track parameters. |
| `Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleTrackParamExtractor.hpp` | Introduced `ParticleTrackParamExtractor` class and `Config` struct for track parameter extraction. |
| `Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TrackParameterSmearing.cpp` | Added `TrackParameterSmearing` class for smearing track parameters based on configuration. |
| `Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TrackParameterSmearing.hpp` | Renamed class from `ParticleSmearing` to `TrackParameterSmearing`, updated `Config` struct parameters. |
| `Examples/Algorithms/TruthTracking/CMakeLists.txt` | Removed `ParticleSmearing.cpp`, added `ParticleTrackParamExtractor.cpp` and `TrackParameterSmearing.cpp`. |
| `Examples/Python/python/acts/examples/reconstruction.py` | Replaced `ParticleSmearingSigmas` with `TrackSmearingSigmas`, updated function signatures. |
| `Examples/Python/src/Generators.cpp` | Added multiple generator classes and refined generator exposure to Python. |
| `Examples/Python/src/TruthTracking.cpp` | Replaced `ParticleSmearing` with `ParticleTrackParamExtractor`, updated parameters. |
| `Examples/Python/tests/conftest.py` | Updated `basic_prop_seq` fixture to use `ParticleTrackParamExtractor`, modified logging levels. |
| `Examples/Python/tests/test_algorithms.py` | Renamed module from `ParticleSmearing` to `TrackParameterSmearing`. |
| `Examples/Python/tests/test_propagation.py` | Updated tests to replace particle smearing with track parameter extraction. |
| `Examples/Scripts/Optimization/ckf.py` | Renamed `ParticleSmearingSigmas` to `TrackSmearingSigmas`, updated parameters. |
| `Examples/Scripts/Python/ckf_tracks.py` | Renamed `ParticleSmearingSigmas` to `TrackSmearingSigmas`, updated parameters. |
| `Examples/Scripts/Python/material_validation.py` | Replaced `ParticleSmearing` with `ParticleTrackParamExtractor`, updated parameters. |
| `Examples/Scripts/Python/material_validation_itk.py` | Added parameters for dynamic configuration, updated extractor instantiation. |
| `Examples/Scripts/Python/propagation.py` | Replaced `ParticleSmearing` with `ParticleTrackParamExtractor`, updated parameters. |
| `Examples/Scripts/Python/vertex_fitting.py` | Updated to use `TrackParameterSmearing` and `ParticleTrackParamExtractor`. |
| `Examples/Scripts/Python/full_chain_test.py` | Updated argument parsing and handling of particle tracking and reconstruction. |

## Possibly related PRs
- **#3836**: Changes related to renaming `ParticleSmearingSigmas` to `TrackSmearingSigmas` are connected to modifications in the `HitSelector` class, enhancing the tracking process.
- **#3886**: This PR modifies the `RootTrackSummaryWriter` to function without truth information, which aligns with the broader context of handling track parameters and configurations in the main PR.

## Suggested labels
`automerge`

> "In code we trust, with names anew,  
> Track smearing sigmas, clear and true.  
> From particles to tracks, we now align,  
> With parameters refined, our paths entwine.  
> New classes emerge, old ones depart,  
> In the realm of code, we play our part!" 🎉

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.

@andiwand andiwand changed the title refactor: Replace ParticleSmearing with TrackParameterSmearing in Examples refactor: Replace ParticleSmearing with TrackParameterSmearing in Examples Nov 8, 2024
@andiwand andiwand marked this pull request as ready for review November 27, 2024 20:32
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: 6

🧹 Outside diff range and nitpick comments (20)
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleTrackParamExtractor.hpp (2)

23-31: More wisdom in documentation share, you must.

Brief, the current documentation is. Enlighten others about:

  • How the extraction process works
  • What parameters are extracted
  • When this algorithm should be used

Apply this improvement, you should:

-/// Extract track parameters from particles.
+/// Extract track parameters from particles by computing the track parameters
+/// at the particle vertex position. This algorithm converts particle information
+/// into track parameters that can be used for track fitting and other tracking
+/// algorithms. The extraction preserves the kinematic properties of the particle
+/// at its production vertex.

1-48: Wise architectural decision, this is.

Separate the concerns, you have:

  • Track parameter extraction stands alone
  • Flexibility for different use cases, you gain
  • Clear responsibility boundaries, you maintain

But remember to:

  • Document the extraction algorithm details in the implementation
  • Consider adding unit tests for edge cases
  • Provide examples for common use cases
Examples/Scripts/Python/material_validation.py (1)

49-58: Document the flow of track parameters, suggest I do!

Clear the configuration is, but document the journey of track parameters from extraction to propagation, we should. Help future Padawans understand the flow, this will.

Add this documentation above the propagation algorithm configuration:

+    # Configure propagation algorithm to use track parameters extracted from particles
+    # The material interactions are recorded for validation purposes
     alg = acts.examples.PropagationAlgorithm(
Examples/Scripts/Python/material_validation_itk.py (1)

Line range hint 13-64: Consider error handling for invalid parameter values, you should.

Missing validation for nevents and ntracks parameters, I observe. Negative or zero values, problematic they could be.

def runMaterialValidation(
    nevents,
    ntracks,
    trackingGeometry,
    decorators,
    field,
    outputDir,
    outputName="propagation_material",
    dumpPropagationSteps=False,
    s=None,
):
+    if nevents <= 0:
+        raise ValueError("Number of events must be positive")
+    if ntracks <= 0:
+        raise ValueError("Number of tracks must be positive")
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TrackParameterSmearing.hpp (2)

47-58: Improved clarity in parameter naming, I sense.

From d0/z0 to loc0/loc1, the parameters have evolved. More general and clear, their purpose has become. But document the units in comments, we should.

Add these clarifying comments:

-    /// Constant term of the loc0 resolution.
+    /// Constant term of the loc0 resolution in the local frame.
+    /// @note Units: micrometers (μm)
     double sigmaLoc0 = 20 * Acts::UnitConstants::um;

Line range hint 1-96: Architecture advice, share I must.

Strong in the Force, this refactoring is. But consider these points, you should:

  1. Template parameters for different smearing strategies, useful they could be
  2. Interface for custom resolution calculations, flexibility it would bring
  3. Validation utilities for parameter bounds, safety they would ensure

The path to maintainable code, these suggestions are.

Examples/Scripts/Python/propagation.py (2)

34-39: Hmmmm, adjust the logging level, you should consider.

Wise the transition to ParticleTrackParamExtractor is, yes. But WARNING level, too silent it might be. For debugging purposes, INFO level more helpful could be, especially in these early days of the new implementation.

-        level=acts.logging.WARNING,
+        level=acts.logging.INFO,

Line range hint 34-53: Strong in the Force, this architectural change is.

Improved the design has become, by separating track parameter extraction from particle smearing, hmm. More flexible and clear, the new approach is. A path to better maintainability and extensibility, you have chosen.

Examples/Scripts/Python/vertex_fitting.py (2)

68-73: Consider temporary logging adjustment, you should.

Wise it is to extract track parameters separately, yes. But in the early days of deployment, more verbose logging might serve us well.

-            level=acts.logging.WARNING,
+            level=acts.logging.INFO,  # Temporary: increase visibility during initial deployment

68-78: A path to future improvements, I sense.

Consider, young Padawan, these architectural suggestions:

  1. A factory pattern for track parameter extraction strategies, useful it could be
  2. Configuration validation for different vertex fitting scenarios, add we should

The Force is strong with this refactoring, but room for growth, there always is.

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

Line range hint 27-45: Complete the transition to track parameters, we must.

In the constructor configuration, track parameter handling setup, I see not. For the path to TrackParameterSmearing, additional configuration options needed they are.

Add this configuration, you should:

 struct Config {
   std::string outputParticles;
   std::string outputVertices;
+  std::string outputTrackParameters;  // Track parameters output collection
   std::vector<Generator> generators;
   std::shared_ptr<RandomNumbers> randomNumbers;
+  std::shared_ptr<TrackParameterExtractor> parameterExtractor;  // Track parameter extraction
 };
Examples/Scripts/Python/ckf_tracks.py (1)

102-110: Wise changes these parameter names are, but documentation strengthen we should.

The transition from particle-specific parameters (d0, z0) to general track parameters (loc0, loc1), a good path this is. Zero values, disable smearing they do, as mentioned in the ancient scrolls of the PR comments.

Add we should, a brief comment explaining:

 TrackSmearingSigmas(  # only used by SeedingAlgorithm.TruthSmeared
+    # loc0, loc1: local track parameters in the measurement frame
+    # time: temporal parameter for the track
+    # phi, theta: angular parameters
+    # ptRel: relative transverse momentum uncertainty
     # zero eveything so the CKF has a chance to find the measurements
     loc0=0,
Examples/Python/src/TruthTracking.cpp (2)

50-54: Strong with the Force, this TrackParameterSmearing implementation is!

Rich in configuration options, this new algorithm is:

  • Comprehensive sigma parameters for various dimensions
  • Particle hypothesis support
  • Random number generation integration

Matches the PR's goal of providing more general track parameter smearing approach, it does.

Consider documenting the meaning and units of each sigma parameter in the header file, help future Padawans it will.


Incomplete migration detected, remaining references to ParticleSmearing there are!

Found references in these files, we have:

  • Examples/Scripts/Python/full_chain_test.py: Still uses ParticleSmearingSigmas
  • Examples/Python/python/acts/examples/reconstruction.py: Contains comments about particle smearing

Update these files to use the new TrackParameterSmearing system, you must. Left behind, these old references should not be. Hmmmm.

🔗 Analysis chain

Line range hint 13-54: Verify the complete transition from ParticleSmearing, we must!

Ensure that all references to the old ParticleSmearing have been properly migrated to the new system, we should.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify complete removal of ParticleSmearing

echo "Searching for any remaining ParticleSmearing references..."
rg "ParticleSmearing" --type cpp --type python

echo "Checking for any TODO comments related to the migration..."
rg "TODO.*[Pp]article.*[Ss]mearing" --type cpp --type python

Length of output: 425


Script:

#!/bin/bash
# Search for ParticleSmearing references with correct file types
echo "Searching for ParticleSmearing references..."
rg "ParticleSmearing" 

echo "Checking specific Python and C++ files in Examples directory..."
fd -e cpp -e hpp -e py -e h . Examples | xargs rg "ParticleSmearing"

echo "Checking for related configuration or test files..."
fd -e json -e yaml -e xml . Examples | xargs rg -i "particle.*smearing"

Length of output: 1345

Examples/Scripts/Optimization/ckf.py (1)

191-199: Documentation improvements suggest, I must!

Clear the intention is, with zeros for all parameters. Yet, young padawan, wisdom shared must be. Add comment explaining why zero values chosen, you should. Help future maintainers understand the ways of the Force, it will.

Apply this change, you should:

 TrackSmearingSigmas(  # only used by SeedingAlgorithm.TruthSmeared
-            # zero eveything so the CKF has a chance to find the measurements
+            # Zero all smearing parameters to allow CKF to find measurements naturally,
+            # without bias from pre-smeared track parameters. This approach lets the
+            # algorithm determine the true measurement-to-track associations.
             loc0=0,
             loc0PtA=0,
             loc0PtB=0,
             loc1=0,
             loc1PtA=0,
             loc1PtB=0,
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleTrackParamExtractor.cpp (1)

42-44: Typo in comment, there is. Remove extraneous 'to', you must.

In the comment, unnecessary word 'to' included, it is. Correct form: 'use it as the reference position', it should be.

Apply this diff to fix the typo:

-// particles within the group originate from the same position and use it
-// to as the reference position for the perigee frame.
+// particles within the group originate from the same position and use it
+// as the reference position for the perigee frame.
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TrackParameterSmearing.cpp (2)

104-105: Use std::hypot for numerical stability, you should.

Compute hypotenuse, std::hypot does. Better numerical stability, it provides. Replace std::sqrt(std::pow(a, 2) + std::pow(b, 2)), you can.

Apply this diff to improve the code:

-        const double sigmaQOverP =
-            std::sqrt(std::pow(m_cfg.sigmaPtRel * qOverP, 2) +
-                      std::pow(sigmaTheta * (qOverP * std::tan(theta)), 2));
+        const double sigmaQOverP =
+            std::hypot(m_cfg.sigmaPtRel * qOverP,
+                       sigmaTheta * (qOverP * std::tan(theta)));

43-52: Consolidate debug logging, helpful it would be.

Many ACTS_DEBUG statements, there are. For clarity and efficiency, combine them, you could.

Apply this diff to enhance logging:

-ACTS_DEBUG("smearing track param loc0 " << m_cfg.sigmaLoc0 << " A "
-                                         << m_cfg.sigmaLoc0PtA << " B "
-                                         << m_cfg.sigmaLoc0PtB);
-ACTS_DEBUG("smearing track param loc1 " << m_cfg.sigmaLoc1 << " A "
-                                         << m_cfg.sigmaLoc1PtA << " B "
-                                         << m_cfg.sigmaLoc1PtB);
-ACTS_DEBUG("smearing track param time " << m_cfg.sigmaTime);
-ACTS_DEBUG("smearing track param phi " << m_cfg.sigmaPhi);
-ACTS_DEBUG("smearing track param theta " << m_cfg.sigmaTheta);
-ACTS_DEBUG("smearing track param q/p " << m_cfg.sigmaPtRel);
+ACTS_DEBUG("Smearing track parameters:"
+           << "\n  loc0: " << m_cfg.sigmaLoc0 << " A " << m_cfg.sigmaLoc0PtA << " B " << m_cfg.sigmaLoc0PtB
+           << "\n  loc1: " << m_cfg.sigmaLoc1 << " A " << m_cfg.sigmaLoc1PtA << " B " << m_cfg.sigmaLoc1PtB
+           << "\n  time: " << m_cfg.sigmaTime
+           << "\n  phi: " << m_cfg.sigmaPhi
+           << "\n  theta: " << m_cfg.sigmaTheta
+           << "\n  q/p: " << m_cfg.sigmaPtRel);
Examples/Python/tests/conftest.py (2)

213-213: Changed logging level to WARNING, intentional this is?

Previously set to INFO it was. Confirm that this change reflects your desired logging verbosity, you should.


215-215: Consistency in naming conventions, ensure you must.

The output parameter name "params_particles_input" is. Verify that it aligns with naming conventions and is consistent throughout the codebase, you should.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 2e74ffa and a45a733.

📒 Files selected for processing (21)
  • CI/physmon/workflows/physmon_trackfinding_1muon.py (2 hunks)
  • Examples/Algorithms/Generators/ActsExamples/Generators/EventGenerator.cpp (3 hunks)
  • Examples/Algorithms/Generators/ActsExamples/Generators/EventGenerator.hpp (1 hunks)
  • Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSmearing.cpp (0 hunks)
  • Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleTrackParamExtractor.cpp (1 hunks)
  • Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleTrackParamExtractor.hpp (1 hunks)
  • Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TrackParameterSmearing.cpp (1 hunks)
  • Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TrackParameterSmearing.hpp (3 hunks)
  • Examples/Algorithms/TruthTracking/CMakeLists.txt (1 hunks)
  • Examples/Python/python/acts/examples/reconstruction.py (7 hunks)
  • Examples/Python/src/Generators.cpp (0 hunks)
  • Examples/Python/src/TruthTracking.cpp (2 hunks)
  • Examples/Python/tests/conftest.py (2 hunks)
  • Examples/Python/tests/test_algorithms.py (2 hunks)
  • Examples/Python/tests/test_propagation.py (1 hunks)
  • Examples/Scripts/Optimization/ckf.py (2 hunks)
  • Examples/Scripts/Python/ckf_tracks.py (2 hunks)
  • Examples/Scripts/Python/material_validation.py (1 hunks)
  • Examples/Scripts/Python/material_validation_itk.py (3 hunks)
  • Examples/Scripts/Python/propagation.py (2 hunks)
  • Examples/Scripts/Python/vertex_fitting.py (2 hunks)
💤 Files with no reviewable changes (2)
  • Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleSmearing.cpp
  • Examples/Python/src/Generators.cpp
✅ Files skipped from review due to trivial changes (1)
  • Examples/Algorithms/Generators/ActsExamples/Generators/EventGenerator.hpp
🧰 Additional context used
🪛 Ruff (0.8.0)
Examples/Python/python/acts/examples/reconstruction.py

257-257: Do not perform function call TrackSmearingSigmas in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable

(B008)

🔇 Additional comments (33)
Examples/Algorithms/TruthTracking/CMakeLists.txt (2)

5-6: Hmmmm, wise changes these are, approve them I do!

Replaced ParticleSmearing with new implementations, you have. Clear and purposeful, the changes are.

Verify the build dependencies, we must:

✅ Verification successful

Verified, the changes are. Approve them with confidence, I do!

Found both new source files in their proper place, we have:

  • Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleTrackParamExtractor.cpp
  • Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TrackParameterSmearing.cpp

No traces of ParticleSmearing in CMake files remain, hmmmm. Clean and complete, the transition is.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if all new source files exist and verify no lingering ParticleSmearing references

# Check if new source files exist
echo "Checking existence of new source files..."
fd "ParticleTrackParamExtractor.cpp|TrackParameterSmearing.cpp" --type f

# Check for any remaining ParticleSmearing references in CMake files
echo "Checking for remaining ParticleSmearing references..."
rg "ParticleSmearing" -g "CMakeLists.txt"

Length of output: 505


5-6: Verify complete removal of old ways, we must!

Check for lingering references to ParticleSmearing in the codebase, essential it is.

Run this verification, we shall:

✅ Verification successful

Completed our investigation has been, young Padawan!

Found only one reference to ParticleSmearing in full_chain_test.py we have, where it exists as ParticleSmearingSigmas in a configuration context. Not a remnant of old implementation this is, but rather a current configuration parameter it remains. No other traces of old ParticleSmearing implementation in the codebase, detected we have.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for any remaining ParticleSmearing references in source files

# Search for ParticleSmearing in source files
echo "Searching for ParticleSmearing in source files..."
rg "ParticleSmearing" --type cpp --type hpp

# Search for include statements
echo "Searching for ParticleSmearing includes..."
rg "#include.*ParticleSmearing" --type cpp --type hpp

Length of output: 359


Script:

#!/bin/bash
# Hmmmm, search differently we must. Use proper file extensions, we shall!

# Search for ParticleSmearing in C++ source and header files
echo "Search for ParticleSmearing references, we shall..."
rg "ParticleSmearing" -t cpp -t h -t hpp -t cxx

# Search in CMake files too, we must
echo "In CMake files, look we will..."
rg "ParticleSmearing" -t cmake

# Check includes with proper extensions
echo "For include statements, search we must..."
rg "#include.*ParticleSmearing" -t cpp -t h -t hpp -t cxx

# Use fd to find any files with ParticleSmearing in name
echo "Files with ParticleSmearing in name, find we shall..."
fd "ParticleSmearing"

Length of output: 612


Script:

#!/bin/bash
# Simpler approach, try we must. The Force guides us to basic commands.

# Search with ripgrep, more basic options we shall use
echo "With simple ripgrep, search we must..."
rg -l "ParticleSmearing"

# Through source directories, look we shall
echo "In source files specifically, seek we will..."
rg -l "ParticleSmearing" Examples/
rg -l "ParticleSmearing" Core/

# All files related to truth tracking, find we must
echo "Truth tracking related files, examine we shall..."
fd . Examples/TruthTracking/

Length of output: 615


Script:

#!/bin/bash
# The Force guides us to examine the Python script we found

# Content of the Python script, examine we must
echo "The content of full_chain_test.py, reveal we shall..."
rg "ParticleSmearing" Examples/Scripts/Python/full_chain_test.py -A 5 -B 5

# All truth tracking files in Examples directory, find we must
echo "Truth tracking files in Examples, locate we shall..."
fd "Truth" Examples/

# The structure of Examples directory, understand we must
echo "Examples directory structure, show us..."
ls -R Examples/

Length of output: 31314

Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleTrackParamExtractor.hpp (1)

11-18: Organized well, the includes are. Hmmmm.

Clean and proper, the header organization is. System headers at the bottom, project headers at the top, balance in all things we maintain.

Examples/Python/tests/test_algorithms.py (2)

17-17: Approve the import statement change, I do!

Correctly aligned with the force of refactoring, this change is. From ParticleSmearing to TrackParameterSmearing, the transition flows naturally, hmmmm.


47-47: Wise decision to update test parameters, this is!

Maintained the test coverage has been, while embracing the new TrackParameterSmearing path. In balance with the force, these changes are, yes.

Examples/Python/tests/test_propagation.py (3)

63-68: Hmmmm, wise addition of track parameter extraction, this is!

Properly configured, the new ParticleTrackParamExtractor is. Clear naming of collections, I see. Aligned with PR objectives of replacing particle smearing, this change is.


74-74: Consistent with the force of track parameters, this change is!

Match the output of track parameter extractor, the input parameter name now does. "params_particles_input" replacing "start_parameters", harmony in the force this brings.


63-79: Verify the flow of track parameters through the force, we must!

Through the sequence of algorithms, track parameters must flow smoothly. Test we must, that extraction and propagation work together in harmony.

✅ Verification successful

Harmonious flow of parameters, I sense. Verified, this integration is!

Clear patterns across the codebase, revealed they are. In all instances where ParticleTrackParamExtractor and PropagationAlgorithm together they work, consistent the parameter flow remains:

  • "params_particles_input" as output from ParticleTrackParamExtractor
  • Same parameter name as input to PropagationAlgorithm

Established pattern this is, in propagation.py, material_validation.py, and your test file follows it well. The Force flows strong through these connections, hmm.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the integration between ParticleTrackParamExtractor and PropagationAlgorithm
# Search for other test files that might be affected by this change

# Look for other test files using PropagationAlgorithm
rg -l "PropagationAlgorithm.*inputTrackParameters" --type py

# Look for other usages of ParticleTrackParamExtractor
rg -l "ParticleTrackParamExtractor" --type py

Length of output: 418


Script:

#!/bin/bash
# Examine the usage patterns in the identified files
echo "=== Checking propagation.py ==="
rg -A 5 "ParticleTrackParamExtractor|PropagationAlgorithm" Examples/Scripts/Python/propagation.py

echo -e "\n=== Checking material_validation.py ==="
rg -A 5 "ParticleTrackParamExtractor|PropagationAlgorithm" Examples/Scripts/Python/material_validation.py

echo -e "\n=== Checking vertex_fitting.py ==="
rg -A 5 "ParticleTrackParamExtractor|PropagationAlgorithm" Examples/Scripts/Python/vertex_fitting.py

echo -e "\n=== Checking test_propagation.py ==="
rg -A 5 "ParticleTrackParamExtractor|PropagationAlgorithm" Examples/Python/tests/test_propagation.py

echo -e "\n=== Checking reconstruction.py ==="
rg -A 5 "ParticleTrackParamExtractor|PropagationAlgorithm" Examples/Python/python/acts/examples/reconstruction.py

Length of output: 3254

Examples/Scripts/Python/material_validation.py (1)

43-48: Hmmmm, wise changes these are, approve them I do!

Replaced ParticleSmearing with ParticleTrackParamExtractor, a more focused path this is. Clear and descriptive, the new parameter names are.

Verify the consistency of these changes across the codebase, we must:

✅ Verification successful

Hmmmm, clear now the path forward is!

The old "start_parameters" references, belong to core fitting algorithms they do. Different context they have, not related to our changes they are. The new parameter name "params_particles_input" conflicts with nothing, wise this choice was!

Let us search deeper for the extractor's usage pattern we must, for incomplete the previous search was. A more specific pattern, use we shall.


Consistent and harmonious, the Force is with these changes!

In balance the codebase is, hmmmm! The ParticleTrackParamExtractor's usage across all Python scripts, consistent it remains. The parameter name "params_particles_input" properly used it is, both as output and input throughout the examples and tests. Matches the pattern in other files it does, including propagation.py, vertex_fitting.py, and material_validation_itk.py.

No disturbance in the Force I sense. Safe these changes are!

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining references to the old parameter name
rg "start_parameters" 

# Verify the new extractor's usage pattern
ast-grep --pattern 'ParticleTrackParamExtractor($$$)'

Length of output: 5349


Script:

#!/bin/bash
# Search for ParticleTrackParamExtractor usage with Python syntax
rg "ParticleTrackParamExtractor\(" --type py -A 3

# Search for the new parameter name to ensure consistency
rg "params_particles_input" --type py

Length of output: 3976

Examples/Scripts/Python/material_validation_itk.py (4)

13-14: Approve the addition of configuration parameters, I do.

Wise changes these are, allowing flexible control over events and tracks. More configurable and reusable, the function becomes.


23-24: Properly utilized the nevents parameter is.

Dynamic configuration through parameter passing, better than hardcoded values it is. Multi-threading enabled with numThreads=-1, scalable performance this brings.


50-55: Verify the track parameter extraction alignment, we must.

Replaced the particle generator with track parameter extractor, addressing the alignment concerns mentioned in PR objectives. But careful we must be, for reliable results we seek.

✅ Verification successful

Consistent and well-aligned, the track parameter extraction configuration is.

Through the Force, I sense harmony in the implementation across the codebase. The same configuration pattern appears in multiple test files and example scripts:

  • In Examples/Python/tests/test_propagation.py
  • In Examples/Python/tests/conftest.py
  • In Examples/Scripts/Python/propagation.py
  • In Examples/Scripts/Python/vertex_fitting.py

All follow the same sacred pattern of configuration with matching input and output parameters. The logging level difference (INFO vs WARNING) matters not, for it affects only the verbosity of messages, not the extraction alignment.

The implementation in material_validation_itk.py mirrors the established wisdom found throughout the codebase. Trust in its alignment, we can.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other usages of ParticleTrackParamExtractor to verify consistent configuration
rg -A 5 "ParticleTrackParamExtractor" 

# Look for any related test files that validate the extraction
fd -g "*test*track*param*.py"

Length of output: 9973


57-64: Consistent with the new parameter naming, the propagation configuration is.

Updated input track parameters to match the extractor's output, maintaining the flow of data it does. But verify the material interactions handling, we should.

✅ Verification successful

The first attempt with ast-grep failed, hmm. A different approach with ripgrep, we shall try. Search for material handling configurations across Python files, we must. Multiple patterns to verify consistency in propagation setups, I seek.


Consistent in the Force, the material handling is, young Padawan

Searched the codebase extensively, I have. Material interaction configurations across all PropagationAlgorithm instances, consistent they are. The following truths, revealed they were:

  • In material validation scripts, recordMaterialInteractions=True always set it is
  • The naming of outputMaterialTracks="material-tracks", a standard pattern it follows
  • The flow of track parameters, properly maintained it is

Through the Force, see no disturbances in the material handling implementation, I do. Consistent with the ways of the code, this change is.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other propagation algorithms to ensure consistent material handling
ast-grep --pattern 'PropagationAlgorithm(
  $$$
  recordMaterialInteractions=$_
  $$$
)'

Length of output: 90


Script:

#!/bin/bash
# Search for material interaction configurations in Python scripts
rg -U "recordMaterialInteractions.*=.*" --type py -A 3

# Search for material tracks output configurations
rg -U "outputMaterialTracks.*=.*" --type py -A 3

# Find all PropagationAlgorithm configurations
rg -U "PropagationAlgorithm\(" --type py -A 10

Length of output: 8330

Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TrackParameterSmearing.hpp (3)

29-35: Approve the class documentation changes, I do.

Clear and precise, the documentation has become. Well explained, the local frame context is. Hmmmm.


38-40: Consistent with our objectives, these changes are.

From particles to track parameters, the transition is complete. But verify the impact across the codebase, we must.


90-91: Handle the data correctly, we now do.

Aligned with the new track parameters approach, these handles are. But verify the container type usage throughout the codebase, we must.

✅ Verification successful

Consistent usage of TrackParametersContainer throughout the codebase, I see.

Searched through the Force, I have. The TrackParametersContainer, properly used it is across many files. In Examples/Framework/include/ActsExamples/EventData/Track.hpp, defined as std::vector it is, and consistently used this way throughout the codebase, including in your file TrackParameterSmearing.hpp.

The handle declaration in your code:

ReadDataHandle<TrackParametersContainer> m_inputTrackParameters{
    this, "InputTrackParameters"};

Aligns perfectly with all other usages in the codebase, it does. No disturbance in the Force, I sense.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for TrackParametersContainer usage patterns
ast-grep --pattern 'TrackParametersContainer' -l

Length of output: 154


Script:

#!/bin/bash
# Search for TrackParametersContainer usage with ripgrep
rg "TrackParametersContainer" -l

# Get more context around TrackParametersContainer usage
rg "TrackParametersContainer" -B 2 -A 2

# Look for similar container types
rg "Parameters.*Container" -l

Length of output: 34620

Examples/Scripts/Python/propagation.py (1)

53-53: Consistent with the Force, this change is. But verify its reach, we must.

The new parameter name "params_particles_input" aligns well with our track parameter extraction approach. Yet, ensure we must that all consumers of these parameters updated they are.

✅ Verification successful

In harmony with the Force, this change flows. Consistent across all Python scripts, it is.

Through the Force, seen I have that "params_particles_input" parameter consistently used it is in all Python examples and tests. The old "start_parameters" references belong to C++ template parameters they do, unrelated to our Python configuration they are.

  • Found in material validation: Examples/Scripts/Python/material_validation.py
  • Found in propagation tests: Examples/Python/tests/test_propagation.py
  • Found in vertex fitting: Examples/Scripts/Python/vertex_fitting.py
  • Found in configuration tests: Examples/Python/tests/conftest.py

Consistent implementation across all Python scripts that use this parameter, there is. No further changes needed, hmm.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining references to the old parameter name
rg "start_parameters" --type python

# Search for all uses of the new parameter name to ensure consistency
rg "params_particles_input" --type python

Length of output: 139


Script:

#!/bin/bash
# Search for any remaining references to the old parameter name, without file type restriction
rg "start_parameters"

# Search for all uses of the new parameter name to ensure consistency
rg "params_particles_input"

# Find all Python files for context
fd "\.py$"

Length of output: 13064

Examples/Scripts/Python/vertex_fitting.py (2)

9-9: Hmmmm, good this change is!

Aligned with our objectives, the import of TrackParameterSmearing is. A more elegant approach to track parameter handling, it provides.


75-78: Strong with the Force, this implementation is.

Correctly configured, the TrackParameterSmearing is. But verify its impact on existing vertex fitting workflows, we must.

✅ Verification successful

Safe to proceed, this change is.

Examined the Force flows, I have. No disturbance in the vertex fitting workflows, do I sense. The TrackParameterSmearing, only in test configurations and the example script itself, appears. Independent of smearing implementation, the vertex_mu_scan.py script remains, hmm. In test_algorithms.py, merely listed in available algorithms, the TrackParameterSmearing is.

Clear path forward, we have. No other vertex fitting implementations affected by this change, there are.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other vertex fitting implementations that might be affected
rg -l "ParticleSmearing.*vertex" --type py

# Check for any remaining ParticleSmearing usage in test files
rg -l "ParticleSmearing.*test" --type py

Length of output: 87


Script:

#!/bin/bash
# Search for any vertex fitting workflows that use track parameter smearing
rg -l "TrackParameterSmearing.*vertex" --type py

# Search for files containing both vertex fitting and smearing
rg -l "vertex.*fitting.*smear|smear.*vertex.*fitting" --type py

# Look for test files that might be affected
fd -e py test | xargs rg -l "TrackParameterSmearing|vertex.*fitting"

# Check for any examples using vertex fitting
fd "vertex.*\.py" Examples/

Length of output: 335


Script:

#!/bin/bash
# Check the content of test_algorithms.py for any potential impacts
rg "TrackParameterSmearing|vertex.*fitting" -C 5 Examples/Python/tests/test_algorithms.py

# Examine vertex_mu_scan.py for any smearing configurations
cat Examples/Scripts/vertex_mu_scan.py

# Look for any direct dependencies on the changed file
rg -l "from.*vertex_fitting import" --type py

Length of output: 2904

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

11-23: Wise additions to the includes, I sense.

Track-related headers and standard utilities, added they are. Align with the path to track parameter handling, these changes do.


25-26: Cleaner code structure achieved, it has been.

Wrapped in ActsExamples namespace, the code now is. Removed redundant namespace prefixes, we have. Easier to read and maintain, the code becomes.

Also applies to: 131-131


Line range hint 54-127: Verify the track parameter extraction, we must.

Hmm, preserved the event generation logic is, but track parameter extraction, I see not. With the migration to TrackParameterSmearing, ensure proper parameter handling, we should.

Run this script to find track parameter extraction implementation, we shall:

Consider extracting track parameters within this class, you should. A separate method for parameter extraction, beneficial it would be.

CI/physmon/workflows/physmon_trackfinding_1muon.py (3)

21-21: Approve this change, I do!

Wise decision it is, to import TrackSmearingSigmas. Aligned with the Force of refactoring, this change is. Hmmmm.


94-102: Clear and meaningful, these new parameter names are!

From cryptic 'd0', 'z0' to descriptive 'loc0', 'loc1' we have moved. More intuitive for young padawans, this naming shall be.


94-102: Verify the impact of zero smearing values, we must.

Set to zero, all smearing parameters are. Though intentional this may be, verify we must that CKF measurements remain unaffected.

✅ Verification successful

Safe and intentional, the zero smearing values are, hmm.

Clear evidence in the codebase, I see. Multiple files show this pattern, they do. A deliberate choice it is, to help CKF find measurements, yes. In ckf_tracks.py, physmon_trackfinding_1muon.py, and other examples, the same configuration pattern appears with explicit comments explaining the purpose:

"zero everything so the CKF has a chance to find the measurements"

Documentation in docs/core/reconstruction/pattern_recognition/track_finding.md confirms that CKF searches for compatible measurements as it propagates, it does. Zero smearing helps this process, yes.

The Force tells me this is the way it should be. No disturbance in the measurements, I sense.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other instances where these parameters might be used
rg -A 5 "TrackSmearingSigmas|ParticleSmearingSigmas" --type python

# Look for any related test configurations
rg -A 10 "def test.*track.*smearing" --type python

Length of output: 183


Script:

#!/bin/bash
# Search for other instances where these parameters might be used, hmm
rg -A 5 "TrackSmearingSigmas|ParticleSmearingSigmas"

# Look for any related test configurations, we shall
rg -A 10 "def test.*track.*smearing"

# Search for CKF measurement related files and configurations
rg -B 2 -A 5 "CKF.*measurement"

# Find all Python files that might contain track finding configurations
fd ".*track.*\.py$"

Length of output: 13910

Examples/Python/src/TruthTracking.cpp (2)

13-13: Hmmmm, proper includes these are, yes!

The force is strong with these new header inclusions. Align with the PR objectives, they do, replacing particle smearing with track parameter handling.

Also applies to: 16-16


45-47: Wise addition of ParticleTrackParamExtractor, this is!

Handle the extraction of track parameters from particles, this algorithm does. Address the alignment challenges mentioned in PR objectives, it will. Clear and focused interface with input particles and output track parameters, I sense.

✅ Verification successful

Proper integration of ParticleTrackParamExtractor, I confirm!

Clear evidence in the codebase, I see:

  • Header and implementation files in Examples/Algorithms/TruthTracking, properly placed they are
  • Constructor validation for input particles, wisely implemented it is
  • Python bindings through ACTS_PYTHON_DECLARE_ALGORITHM, correctly exposed they are

Hmmmm, yes. Strong with the Force, this implementation is. Proper error handling and clean interface, it maintains. No disturbance in the Force, I sense.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the integration of ParticleTrackParamExtractor
# Search for any existing usages to ensure proper migration

rg "ParticleTrackParamExtractor" --type cpp -A 2

Length of output: 3133

Examples/Scripts/Optimization/ckf.py (2)

126-126: Approve this change, I do!

Wise decision it is, to import TrackSmearingSigmas. Align with the force of track parameter handling, it does. Hmmmm.


193-199: Verify the parameter renaming across the galaxy, we must!

Changed the parameter names have been, from d0/z0 to loc0/loc1. Ensure consistent changes across all files, we should.

Run this command, you shall:

✅ Verification successful

Complete and consistent, the parameter renaming is, young Padawan!

Through the Force, I sense that all files now use the new naming convention of loc0/loc1. No traces of the old d0/z0 parameters remain, and the changes are consistent across:

  • Examples/Python/python/acts/examples/reconstruction.py
  • CI/physmon/workflows/physmon_trackfinding_1muon.py
  • Examples/Scripts/Optimization/ckf.py
  • Examples/Scripts/Python/ckf_tracks.py

In harmony, the codebase is. No further changes required, there are.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining uses of old parameter names
echo "Searching for old parameter names..."
rg -l 'd0PtA|d0PtB|z0PtA|z0PtB' --type py

# Search for new parameter usage for consistency
echo "Verifying new parameter naming..."
rg -l 'loc0PtA|loc0PtB|loc1PtA|loc1PtB' --type py

Length of output: 436


Script:

#!/bin/bash
# Search for context around the parameter usage
echo "Examining parameter usage in affected files..."
rg -C 5 'loc[01](PtA|PtB)?' Examples/Scripts/Python/ckf_tracks.py Examples/Scripts/Optimization/ckf.py Examples/Python/python/acts/examples/reconstruction.py CI/physmon/workflows/physmon_trackfinding_1muon.py

# Check if there are any mixed usages of old and new parameter names
echo "Checking for mixed parameter naming..."
rg -C 5 '(d0|z0)(PtA|PtB)?' Examples/Scripts/Python/ckf_tracks.py Examples/Scripts/Optimization/ckf.py Examples/Python/python/acts/examples/reconstruction.py CI/physmon/workflows/physmon_trackfinding_1muon.py

Length of output: 14702

Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/ParticleTrackParamExtractor.cpp (1)

34-70: Well implemented, this execute method is.

Correctly extracts track parameters from particles, the code does. Efficient and clear, your implementation is.

Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TrackParameterSmearing.cpp (1)

107-107: Initialize params unnecessarily, you may.

Overwrite all components, you do. Initializing to zero, unnecessary it might be.

Examples/Python/tests/conftest.py (2)

212-217: Implemented the ParticleTrackParamExtractor, well you have.

Transition from ParticleSmearing to ParticleTrackParamExtractor aligns with the objectives of this pull request.


228-228: Input track parameters updated appropriately, they are.

Passing "params_particles_input" to PropagationAlgorithm ensures correct data flow between algorithms.

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/Scripts/Python/full_chain_test.py (1)

550-551: Document the choice of ptRel value, you should!

A comment explaining why 0.01 (1%) for ptRel chosen was, helpful it would be. Future padawans, understand this decision better, they will.

Add this comment above the configuration:

+                # Use 1% relative pT uncertainty for track parameter smearing
                 trackSmearingSigmas=TrackSmearingSigmas(ptRel=0.01),
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between a45a733 and ce805e8.

📒 Files selected for processing (2)
  • Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TrackParameterSmearing.cpp (1 hunks)
  • Examples/Scripts/Python/full_chain_test.py (2 hunks)
🔇 Additional comments (5)
Examples/Scripts/Python/full_chain_test.py (2)

524-524: Approve the import change, I do!

Wise decision to rename ParticleSmearingSigmas to TrackSmearingSigmas, it is. Aligns with the broader vision of track parameter-based smearing, this change does.


524-524: Search for remnants of the old ways, we must!

Verify that no references to ParticleSmearingSigmas remain in the codebase, we should.

Also applies to: 550-551

✅ Verification successful

Let me search deeper into the Force to find these track smearing references, hmm.


Clear, the path forward is!

No remnants of ParticleSmearingSigmas in the codebase, I sense. Consistently used, the new TrackSmearingSigmas is, across all Python scripts and C++ implementations. In harmony with the Force, these changes are!

  • In Python scripts (ckf.py, ckf_tracks.py, full_chain_test.py), properly used TrackSmearingSigmas is
  • In core implementation (acts/examples/reconstruction.py), correctly defined and utilized, the TrackSmearingSigmas type is
  • In C++ realm (TrackParameterSmearing.hpp), aligned with the new ways, the implementation remains
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining references to ParticleSmearingSigmas
echo "Searching for old ParticleSmearingSigmas references..."
rg "ParticleSmearingSigmas"

# Search for any potential imports or configurations that might need updating
echo "Checking for related smearing configurations..."
rg -A 5 "smearing.*sigmas"

Length of output: 1851


Script:

#!/bin/bash
# Search for any references to TrackSmearingSigmas
echo "Searching for TrackSmearingSigmas references..."
rg "TrackSmearingSigmas"

# Search for any configuration files or test files related to track smearing
echo "Checking for track smearing configurations..."
fd -e py -e hpp -e h ".*[Tt]rack.*[Ss]mear.*" --exec rg -l "Sigmas"

Length of output: 1807

Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TrackParameterSmearing.cpp (3)

108-108: Initialize params vector unnecessary, it may be.

All components you overwrite later. Redundant, the zero initialization is.


141-149: Ensure loop indices within bounds, you must.

From Acts::eBoundLoc0 to Acts::eBoundSize, the loop runs. Valid range, confirm you should, to avoid accessing beyond array limits.


105-106: Potential numerical instability in sigmaQOverP calculation, there is.

When theta near π⁄2 is, tan(theta) large becomes. Numerical instability, this may cause. Ensure theta within valid range is, or handle this case, you should.

Run the following script to identify instances where theta close to π⁄2 is:

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.

  • Looks good.
  • Sonar shows two issues that seem legitimate.
  • Output changes are expected I suppose?

paulgessinger
paulgessinger previously approved these changes Nov 28, 2024
@andiwand
Copy link
Contributor Author

Output changes are expected because the p / q/p calculations change slightly.

I had a look at the sonar issues. I would prefer the explicit type for the distributions over using std defaults. The override thing looks very minor. I would prefer to sweep that out in one go.

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/Python/tests/test_propagation.py (1)

79-79: Additional assertions, suggest I do!

Strong in testing, this code is. Yet stronger it could become. Consider adding assertions to verify the quality of extracted track parameters, we should. Important this is, as mentioned in the PR comments about alignment challenges.

Add assertions like this, you could:

def verify_track_params(ctx):
    params = ctx.eventStore.get("params_particles_input")
    # Verify track parameters are within expected ranges
    for param in params:
        assert param.loc0 is not None, "loc0 parameter missing it is"
        assert abs(param.loc0) < max_expected_value, "Beyond bounds, loc0 parameter is"
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b391f89 and 18266a9.

📒 Files selected for processing (1)
  • Examples/Python/tests/test_propagation.py (1 hunks)
🔇 Additional comments (2)
Examples/Python/tests/test_propagation.py (2)

74-74: Changed parameter name, clarity it brings!

From "start_parameters" to "params_particles_input", the path now clearer it is. Matches the output of our new extractor, it does.


63-68: Hmmmm, good changes these are, but verify we must!

Replaced ParticleSmearing with ParticleTrackParamExtractor, you have. Wise choice this is, aligning with our mission to handle track parameters more generally. But verify the extraction results, we should.

✅ Verification successful

Successful transition to TrackParameterSmearing, I sense

Searched through the Force of the codebase, I have. No traces of ParticleSmearing remain, while TrackParameterSmearing's presence strong it is. The change in test_propagation.py, aligned with the greater purpose it is. In vertex_fitting.py and test_algorithms.py, TrackParameterSmearing already flows, showing consistency across the codebase, it does.

The Force is strong with this change. Trust in it, we can.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining ParticleSmearing references
rg "ParticleSmearing" --type python

Length of output: 66


Script:

#!/bin/bash
# Search for any remaining ParticleSmearing references without file type restriction
rg "ParticleSmearing"

# Search for files containing both ParticleSmearing and TrackParameterSmearing
rg "TrackParameterSmearing"

# Find all Python test files that might contain related changes
fd "test.*\.py$"

Length of output: 2788

@kodiakhq kodiakhq bot merged commit 9c9444e into acts-project:main Nov 29, 2024
44 checks passed
@andiwand andiwand deleted the ex-refactor-track-param-smearing branch November 29, 2024 10:20
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 Fails Athena tests This PR causes a failure in the Athena tests Infrastructure Changes to build tools, continous integration, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants