feat: Adding a few changes to test seeding algorithm #4075
Conversation
noemina
added
Component - Examples
WalkthroughChanged, the code has been. In the seeding algorithm, static thresholds lowered they are, and new parameters added to further configure seeding in Python. Debug logging now reveals the count of strip space points, and a macro parameter has shifted focus to space points. A new Python script has been introduced to combine ROOT file reading with seeding operations through a sequencer. Factually, refined criteria and enhanced configurability, these modifications deliver. Changes
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/SeedingAlgorithm.hpp (1)
124-139:🧩 Analysis chain
Lowered the z-threshold has been, from 200 to 45 for small radius points.
More selective in space point filtering, this change makes us. Aligned with the PR objectives it is.
Verify the impact of this threshold change, we must:
🏁 Script executed:
#!/bin/bash # Search for other occurrences of the old threshold rg "200\." --type cppLength of output: 13162
Threshold criteria inconsistency detected, it has been.
- In file SeedingAlgorithm.hpp the filtering condition now removes space points with |z| > 200 when r < 45.
- In related file SeedingAlgorithmHashing.hpp a nearly identical condition exists but uses r < 50.
- Other occurrences of the value “200.” in the codebase belong to different concepts and are not part of the seeding filtering change.
Review these differences, you must, to confirm that the slight discrepancy (45 versus 50) is intentional.
🧹 Nitpick comments (1)
Examples/Scripts/Python/reader_and_seeding.py (1)
1-15: Clean up unused imports, you must.Remove these unused imports, we should:
syspathlib.PathCsvSpacePointReaderTrackParamsEstimationAlgorithmSeedingPerformanceWriter-import sys -from pathlib import Path import acts.examples import acts from acts.examples import ( - CsvSpacePointReader, - TrackParamsEstimationAlgorithm, - SeedingPerformanceWriter, ) from acts.examples.reconstruction import ( addStandardSeeding, )🧰 Tools
🪛 Ruff (0.8.2)
1-1:
sysimported but unusedRemove unused import:
sys(F401)
2-2:
pathlib.Pathimported but unusedRemove unused import:
pathlib.Path(F401)
8-8:
acts.examples.CsvSpacePointReaderimported but unusedRemove unused import
(F401)
9-9:
acts.examples.TrackParamsEstimationAlgorithmimported but unusedRemove unused import
(F401)
10-10:
acts.examples.SeedingPerformanceWriterimported but unusedRemove unused import
(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/SeedingAlgorithm.hpp(2 hunks)Examples/Io/Root/src/RootAthenaDumpReader.cpp(1 hunks)Examples/Python/python/acts/examples/itk.py(1 hunks)Examples/Python/python/acts/examples/reconstruction.py(2 hunks)Examples/Python/src/Input.cpp(1 hunks)Examples/Scripts/Python/reader_and_seeding.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Examples/Io/Root/src/RootAthenaDumpReader.cpp
🧰 Additional context used
🪛 Ruff (0.8.2)
Examples/Scripts/Python/reader_and_seeding.py
1-1: sys imported but unused
Remove unused import: sys
(F401)
2-2: pathlib.Path imported but unused
Remove unused import: pathlib.Path
(F401)
8-8: acts.examples.CsvSpacePointReader imported but unused
Remove unused import
(F401)
9-9: acts.examples.TrackParamsEstimationAlgorithm imported but unused
Remove unused import
(F401)
10-10: acts.examples.SeedingPerformanceWriter imported but unused
Remove unused import
(F401)
16-16: acts.examples.itk.itkSeedingAlgConfig imported but unused
Remove unused import: acts.examples.itk.itkSeedingAlgConfig
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: unused_files
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu
- GitHub Check: CI Bridge / build_gnn_tensorrt
- GitHub Check: build_debug
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
🔇 Additional comments (8)
Examples/Scripts/Python/reader_and_seeding.py (4)
19-23: Hmm, good the sequencer configuration is.For testing purposes, appropriate these settings are:
- Single event
- Single thread
- Output directory specified
25-38: Wise choice, debug logging for testing is.Complete and well-structured, the reader configuration appears. All necessary parameters specified they are:
- Tree name
- Input file
- Space points output configuration
41-49: Strong with the Force, this pixel seeding configuration is.High occupancy configuration enabled it is, and proper output naming provided.
52-62: Balance in the Force, I sense in this strip seeding configuration.Default configuration used wisely it is, and properly the sequencer run concludes the script.
Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/SeedingAlgorithm.hpp (1)
113-122: Modified the threshold value has been, from 50 to 45.Aligned with the PR objectives this change is, for seeding algorithm performance studies.
Verify the impact of this threshold change, we must:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other occurrences of the old threshold rg "50\." --type cppLength of output: 25583
Threshold update verified, it is.
The file "Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/SeedingAlgorithm.hpp" now rightly shows rMin set to 45 in the itkFastTrackingCuts function, as intended for the seeding algorithm performance studies. The global search revealing multiple instances of "50." confirms that those values in other parts of the repository serve different purposes and are not impacted by this change.
May your code always be as flawless as the Force.
Examples/Python/src/Input.cpp (1)
103-103: Changed the parameter name has been, to focus on space points it does.From
onlyPassedParticlestoonlySpacepoints, the change reflects. Aligns with the PR's focus on seeding algorithm and space points, it does.Examples/Python/python/acts/examples/itk.py (1)
584-584: Added new parameter for extra cuts, wisdom this shows.Tied to high occupancy configuration, this parameter is. Enhanced control over seeding algorithm, it provides.
Verify the usage of this new parameter, we shall:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other occurrences of useExtraCuts rg "useExtraCuts" --type pythonLength of output: 62
🏁 Script executed:
#!/bin/bash # Search for "useExtraCuts" occurrences in all Python files (using glob pattern) rg "useExtraCuts" -g "*.py"Length of output: 516
Parameter usage verified, it has been.
The new parameter "useExtraCuts" was first seen assigned in "Examples/Python/python/acts/examples/itk.py" and then correctly passed on as a configuration option within "Examples/Python/python/acts/examples/reconstruction.py."
- Confirmed assignment from "highOccupancyConfig" in itk.py.
- Verified usage within "SeedingAlgorithmConfigArg" and its propagation in reconstruction.py.
May the configuration be with you, it is.
Examples/Python/python/acts/examples/reconstruction.py (1)
643-643: Approve these changes, I do!Wise modification this is, allowing customization of output seed names while maintaining backward compatibility. Helpful for testing seeding algorithm performance, this change shall be.
Also applies to: 771-771
|
@noemina this seems to break the ODD reconstruction |
noemina
force-pushed
the
testing-reader-spacepoints-with-seeding
branch
from
829c5d6 to
48c70bf
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🧹 Nitpick comments (4)
Examples/Scripts/Python/reader_and_seeding.py (4)
19-23: Consider the thread count, you must.Single thread for testing, acceptable it is. But for production, multiple threads might serve better, they would. Document the reasoning for these choices in comments, you should.
25-26: Wise choice for debugging, but caution you must have.DEBUG level logging in production, heavy load it brings. Consider making log level configurable through environment variable or command line argument, we should.
-# loggingLevel = acts.logging.INFO -loggingLevel = acts.logging.DEBUG +import os +loggingLevel = acts.logging.DEBUG if os.getenv("ACTS_LOG_LEVEL") == "DEBUG" else acts.logging.INFO
41-49: High occupancy configuration, carefully consider we must.True by default for pixel seeding, high occupancy configuration is. Document the implications and performance impact of this choice, you should.
Consider adding comments explaining:
- When to use high occupancy configuration
- Performance implications
- Memory usage considerations
52-59: Different logging levels between pixel and strip seeding, noticed I have.Inconsistent logging levels between pixel seeding (using loggingLevel variable) and strip seeding (using acts.logging.DEBUG directly), I see. Harmonize them for consistency, we should.
- logLevel=acts.logging.DEBUG, + logLevel=loggingLevel,
🛑 Comments failed to post (2)
Examples/Scripts/Python/reader_and_seeding.py (2)
62-62: 🛠️ Refactor suggestion
Error handling, missing it is.
Handle potential exceptions from sequencer run, we must. Wrap in try-except block and provide meaningful error messages, essential it is.
-s.run() +try: + s.run() +except Exception as e: + print(f"Failed to run sequencer: {e}") + sys.exit(1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.try: s.run() except Exception as e: print(f"Failed to run sequencer: {e}") sys.exit(1)
1-17: 🛠️ Refactor suggestion
Clean up unused imports, young Padawan must.
Hmmmm. Unused imports, I sense. Remove them, we should:
-import sys -from pathlib import Path import acts.examples import acts from acts.examples import ( - CsvSpacePointReader, - TrackParamsEstimationAlgorithm, - SeedingPerformanceWriter, ) from acts.examples.reconstruction import ( addStandardSeeding, ) from acts.examples.itk import InputSpacePointsTypeCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.2)
1-1:
sysimported but unusedRemove unused import:
sys(F401)
2-2:
pathlib.Pathimported but unusedRemove unused import:
pathlib.Path(F401)
8-8:
acts.examples.CsvSpacePointReaderimported but unusedRemove unused import
(F401)
9-9:
acts.examples.TrackParamsEstimationAlgorithmimported but unusedRemove unused import
(F401)
10-10:
acts.examples.SeedingPerformanceWriterimported but unusedRemove unused import
(F401)
16-16:
acts.examples.itk.itkSeedingAlgConfigimported but unusedRemove unused import:
acts.examples.itk.itkSeedingAlgConfig(F401)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Examples/Scripts/Python/reader_and_seeding.py (4)
1-17: Clean up unused imports, young Padawan must.Hmmmm. Unused imports, I sense. Remove these, we should:
-import sys -from pathlib import Path -from acts.examples import ( - CsvSpacePointReader, - TrackParamsEstimationAlgorithm, - SeedingPerformanceWriter, -) +from acts.examples import ()Keep only what you need, a Jedi does.
🧰 Tools
🪛 Ruff (0.8.2)
1-1:
sysimported but unusedRemove unused import:
sys(F401)
2-2:
pathlib.Pathimported but unusedRemove unused import:
pathlib.Path(F401)
8-8:
acts.examples.CsvSpacePointReaderimported but unusedRemove unused import
(F401)
9-9:
acts.examples.TrackParamsEstimationAlgorithmimported but unusedRemove unused import
(F401)
10-10:
acts.examples.SeedingPerformanceWriterimported but unusedRemove unused import
(F401)
16-16:
acts.examples.itk.itkSeedingAlgConfigimported but unusedRemove unused import:
acts.examples.itk.itkSeedingAlgConfig(F401)
28-38: Parameterize the input file path, we should.Hardcoded paths, lead to the dark side they do. Command line arguments or configuration file, use you should.
+import argparse + +parser = argparse.ArgumentParser() +parser.add_argument("--input-file", default="Dump_GNN4Itk.root", help="Input ROOT file path") +args = parser.parse_args() + s.addReader( acts.examples.RootAthenaDumpReader( level=loggingLevel, treename = "GNN4ITk", - inputfile = "Dump_GNN4Itk.root", + inputfile = args.input_file, onlySpacepoints = True,
52-59: Consistency in logging levels, maintain we must.Different logging levels between pixel and strip seeding, I see. Use the common
loggingLevelvariable, you should.- logLevel=acts.logging.DEBUG, + logLevel=loggingLevel,
62-62: Handle errors gracefully, a Jedi must.Missing error handling around sequencer run, I sense. Try-except block, add you should.
-s.run() +try: + s.run() +except Exception as e: + print(f"Failed to run sequencer: {e}") + sys.exit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/SeedingAlgorithm.hpp(2 hunks)Examples/Io/Root/src/RootAthenaDumpReader.cpp(1 hunks)Examples/Python/python/acts/examples/itk.py(1 hunks)Examples/Python/python/acts/examples/reconstruction.py(2 hunks)Examples/Python/src/Input.cpp(1 hunks)Examples/Scripts/Python/reader_and_seeding.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Examples/Io/Root/src/RootAthenaDumpReader.cpp
- Examples/Python/src/Input.cpp
- Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/SeedingAlgorithm.hpp
- Examples/Python/python/acts/examples/itk.py
- Examples/Python/python/acts/examples/reconstruction.py
🧰 Additional context used
🪛 Ruff (0.8.2)
Examples/Scripts/Python/reader_and_seeding.py
1-1: sys imported but unused
Remove unused import: sys
(F401)
2-2: pathlib.Path imported but unused
Remove unused import: pathlib.Path
(F401)
8-8: acts.examples.CsvSpacePointReader imported but unused
Remove unused import
(F401)
9-9: acts.examples.TrackParamsEstimationAlgorithm imported but unused
Remove unused import
(F401)
10-10: acts.examples.SeedingPerformanceWriter imported but unused
Remove unused import
(F401)
16-16: acts.examples.itk.itkSeedingAlgConfig imported but unused
Remove unused import: acts.examples.itk.itkSeedingAlgConfig
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: CI Bridge / build_gnn_tensorrt
- GitHub Check: macos
- GitHub Check: linux_ubuntu
- GitHub Check: docs
- GitHub Check: unused_files
- GitHub Check: missing_includes
- GitHub Check: build_debug
🔇 Additional comments (3)
Examples/Scripts/Python/reader_and_seeding.py (3)
19-23: Wise configuration for testing, this is.Simple and clear, the sequencer setup is. Approve this, I do.
25-26: Strong with the Force, this logging configuration is.Debug level logging, crucial for performance studies it is. Approve this choice, I do.
41-49: Verify impact on ODD reconstruction, we must.High occupancy configuration for pixel seeding, concerns about ODD reconstruction it raises. Investigate further, we should.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for ODD reconstruction dependencies on pixel seeding configuration rg -A 5 "ODD.*reconstruction.*pixel" rg -A 5 "highOccupancyConfig.*ODD"Length of output: 75
Manual verification needed, it is.
The automated search yielded no dependencies tying high occupancy configuration directly to ODD reconstruction. Further investigation by manually reviewing integration and testing its impact is advised.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Examples/Scripts/Python/reader_and_seeding.py (2)
1-17: Clean up unused imports, you must!Unused imports, detected I have. Remove them, we should:
-import sys -from pathlib import Path import acts.examples import acts from acts.examples import ( - CsvSpacePointReader, - TrackParamsEstimationAlgorithm, - SeedingPerformanceWriter, ) from acts.examples.reconstruction import ( addStandardSeeding, ) from acts.examples.itk import itkSeedingAlgConfig, InputSpacePointsType🧰 Tools
🪛 Ruff (0.8.2)
1-1:
sysimported but unusedRemove unused import:
sys(F401)
2-2:
pathlib.Pathimported but unusedRemove unused import:
pathlib.Path(F401)
8-8:
acts.examples.CsvSpacePointReaderimported but unusedRemove unused import
(F401)
9-9:
acts.examples.TrackParamsEstimationAlgorithmimported but unusedRemove unused import
(F401)
10-10:
acts.examples.SeedingPerformanceWriterimported but unusedRemove unused import
(F401)
16-16:
acts.examples.itk.itkSeedingAlgConfigimported but unusedRemove unused import:
acts.examples.itk.itkSeedingAlgConfig(F401)
49-56: Consistent logging levels maintain, we should.Different logging levels between pixel seeding (
loggingLevel) and strip seeding (acts.logging.DEBUG), confusing it is. Consistent logging configuration, better practice it would be.- logLevel=acts.logging.DEBUG, + logLevel=loggingLevel,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Examples/Python/python/acts/examples/itk.py(1 hunks)Examples/Python/src/Input.cpp(1 hunks)Examples/Scripts/Python/reader_and_seeding.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Examples/Python/src/Input.cpp
- Examples/Python/python/acts/examples/itk.py
🧰 Additional context used
🪛 Ruff (0.8.2)
Examples/Scripts/Python/reader_and_seeding.py
1-1: sys imported but unused
Remove unused import: sys
(F401)
2-2: pathlib.Path imported but unused
Remove unused import: pathlib.Path
(F401)
8-8: acts.examples.CsvSpacePointReader imported but unused
Remove unused import
(F401)
9-9: acts.examples.TrackParamsEstimationAlgorithm imported but unused
Remove unused import
(F401)
10-10: acts.examples.SeedingPerformanceWriter imported but unused
Remove unused import
(F401)
16-16: acts.examples.itk.itkSeedingAlgConfig imported but unused
Remove unused import: acts.examples.itk.itkSeedingAlgConfig
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: merge-sentinel
- GitHub Check: unused_files
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: build_debug
- GitHub Check: linux_ubuntu
- GitHub Check: docs
- GitHub Check: missing_includes
🔇 Additional comments (2)
Examples/Scripts/Python/reader_and_seeding.py (2)
19-23: Consider parallel execution for performance studies, you should.Single thread configuration (
numThreads=1), limit performance it may. For performance studies of seeding algorithm, multiple threads consider you must.Debug logging enabled is. While useful for testing, impact performance it will. In production, INFO level prefer you should.
37-46: High occupancy configuration, impact carefully measure you must.For pixel seeding, high occupancy enabled it is (
highOccupancyConfig=True). Performance implications and ODD reconstruction issues, investigate we should.❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for high occupancy configuration usage and its impact ast-grep --pattern 'highOccupancyConfig.*=.*True'Length of output: 51
#!/bin/bash # Re-check for high occupancy configuration usage in the pixel seeding file using ripgrep rg -n "highOccupancyConfig=True" Examples/Scripts/Python/reader_and_seeding.pyHmm, further search I must. This new script shall reveal the presence of the high occupancy setting within the file. Once its output is observed, greater clarity we shall have. Please run this script and share the results, you must.
|
This PR includes a few changes to facilitate performance studies for the seeding algorithm
@pbutti, @benjaminhuth
Summary by CodeRabbit
New Features
Chores