refactor(Examples): Replace the drift circle & simHit EDM by the MuonSpacePoint & MuonSegmentEDM #4180
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA transformation in the muon data handling, there is. Drift circles and simulation hits, obsolete they have become. Instead, muon truth segments and space points now feed the Hough seeder, along with new output for Hough maxima. New EDM classes for muon segments, space points, and Hough maximum are introduced while legacy classes and CSV readers are removed or renamed. The Python interface and CMake configuration, updated they are, to reflect the new structure and data flow. Changes
Sequence Diagram(s)sequenceDiagram
participant CSV_Seg as CsvMuonSegmentReader
participant CSV_Space as CsvMuonSpacePointReader
participant Seeder as MuonHoughSeeder
participant Store as EventStore
CSV_Seg->>Seeder: Provide truth segments
CSV_Space->>Seeder: Provide space points
Seeder->>Seeder: Process Hough transform
Seeder->>Store: Output Hough maxima
Suggested labels
Poem
🪧 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 (
|
github-actions
bot
added
Component - Examples
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
Examples/Io/Csv/src/CsvMuonSegmentReader.cpp (2)
52-55: Handle missing file gracefully, one might.
When the file absent is, yes, an error or warning might help. Consider more diagnostic logs or structured error handling.
87-90: Confirm numeric ranges, you should.
Time, timeError, and hits data wise they are. Validate these values come from valid CSV entries, you could.Examples/Io/Csv/src/CsvMuonSpacePointReader.cpp (1)
47-52: Check CSV existence further, one might.
Logging or graceful handling if file not found, consider you could.Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvMuonSpacePointReader.hpp (1)
18-18: Mismatch in comment, I sense.
SimHit reference it still shows, but a SpacePoint type we now read. Update to avoid confusion, you should.Examples/Algorithms/TrackFinding/src/MuonHoughSeeder.cpp (1)
88-322: Greatly expanded theexecutemethod is, handle muon space points it does.
Implementation robust but large, yes. Consider factoring out smaller subroutines, to maintain readability. Updated usage of multiple Hough transform planes, well integrated it appears.Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/MuonHoughSeeder.hpp (1)
39-43: Outdated comments in class description need updating, they do.Still references "CSV files with muon sim hits" and "drift circles" in documentation, even though implementation now uses segments and space points. Update comments to reflect new implementation, you should.
-/// Reads CSV files with muon sim hits (= true trajectories) -/// and drift circles (= measurements), performs -/// a hough transform to the drift circles in each station, +/// Reads muon truth segments (= true trajectories) +/// and space points (= measurements), performs +/// a hough transform to the space points in each station,Examples/Framework/include/ActsExamples/EventData/MuonSegment.hpp (1)
90-95: Inconsistent use of float and double literals, harmonize you should.Some members use 0.f suffix while others use 0. without suffix. For consistency, either use double literals for all or float literals for all.
- double m_time{0.f}; - double m_timeError{0.f}; - double m_chiSquared{0.f}; + double m_time{0.}; + double m_timeError{0.}; + double m_chiSquared{0.};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/MuonHoughSeeder.hpp(3 hunks)Examples/Algorithms/TrackFinding/src/MuonHoughSeeder.cpp(2 hunks)Examples/Framework/CMakeLists.txt(1 hunks)Examples/Framework/include/ActsExamples/EventData/DriftCircle.hpp(0 hunks)Examples/Framework/include/ActsExamples/EventData/MuonHoughMaximum.hpp(1 hunks)Examples/Framework/include/ActsExamples/EventData/MuonSegment.hpp(1 hunks)Examples/Framework/include/ActsExamples/EventData/MuonSimHit.hpp(0 hunks)Examples/Framework/include/ActsExamples/EventData/MuonSpacePoint.hpp(1 hunks)Examples/Framework/src/EventData/MuonSpacePoint.cpp(1 hunks)Examples/Io/Csv/CMakeLists.txt(1 hunks)Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvMuonSegmentReader.hpp(4 hunks)Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvMuonSpacePointReader.hpp(3 hunks)Examples/Io/Csv/src/CsvDriftCircleReader.cpp(0 hunks)Examples/Io/Csv/src/CsvMuonSegmentReader.cpp(1 hunks)Examples/Io/Csv/src/CsvMuonSimHitReader.cpp(0 hunks)Examples/Io/Csv/src/CsvMuonSpacePointReader.cpp(1 hunks)Examples/Io/Csv/src/CsvOutputData.hpp(1 hunks)Examples/Python/src/Input.cpp(2 hunks)Examples/Python/src/TrackFinding.cpp(1 hunks)Examples/Scripts/Python/muon_hough.py(2 hunks)
💤 Files with no reviewable changes (4)
- Examples/Framework/include/ActsExamples/EventData/DriftCircle.hpp
- Examples/Io/Csv/src/CsvDriftCircleReader.cpp
- Examples/Framework/include/ActsExamples/EventData/MuonSimHit.hpp
- Examples/Io/Csv/src/CsvMuonSimHitReader.cpp
🧰 Additional context used
🧬 Code Definitions (4)
Examples/Scripts/Python/muon_hough.py (5)
Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvMuonSpacePointReader.hpp (1)
CsvMuonSpacePointReader(43-43)Examples/Io/Csv/src/CsvMuonSpacePointReader.cpp (1)
CsvMuonSpacePointReader(22-36)Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvMuonSegmentReader.hpp (1)
CsvMuonSegmentReader(50-50)Examples/Io/Csv/src/CsvMuonSegmentReader.cpp (1)
CsvMuonSegmentReader(26-41)Examples/Algorithms/TrackFinding/src/MuonHoughSeeder.cpp (1)
MuonHoughSeeder(67-84)
Examples/Python/src/Input.cpp (4)
Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvMuonSegmentReader.hpp (1)
CsvMuonSegmentReader(50-50)Examples/Io/Csv/src/CsvMuonSegmentReader.cpp (1)
CsvMuonSegmentReader(26-41)Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvMuonSpacePointReader.hpp (1)
CsvMuonSpacePointReader(43-43)Examples/Io/Csv/src/CsvMuonSpacePointReader.cpp (1)
CsvMuonSpacePointReader(22-36)
Examples/Algorithms/TrackFinding/src/MuonHoughSeeder.cpp (1)
Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/MuonHoughSeeder.hpp (3)
MuonHoughSeeder(58-58)m_cfg(69-69)ctx(64-64)
Examples/Framework/include/ActsExamples/EventData/MuonSpacePoint.hpp (2)
Examples/Framework/include/ActsExamples/EventData/MuonSegment.hpp (8)
m_id(40-40)m_time(42-42)id(64-64)id(64-64)pos(54-57)pos(54-54)pos(59-62)pos(59-59)Examples/Framework/src/EventData/MuonSpacePoint.cpp (4)
ostr(15-21)ostr(15-16)ostr(22-28)ostr(22-23)
🔇 Additional comments (50)
Examples/Io/Csv/CMakeLists.txt (1)
18-19: New CSV Reader Files Integration, Commendable It Is!The new CSV readers for MuonSpacePoint and MuonSegment are now added, hmmm. In alignment with the refactoring of the muon EDM, these changes bring much promise. However, ensure that all obsolete CSV readers for drift circles and sim hits are fully removed from the build, they must be. Thorough integration testing, you should perform.
Examples/Framework/CMakeLists.txt (1)
8-8: LGTM - Added source file of wisdom!The path
src/EventData/MuonSpacePoint.cppnow in our library, hmm? Proper this is, for supporting the new MuonSpacePoint EDM that replaces drift circles.Examples/Python/src/TrackFinding.cpp (1)
184-186: Updated algorithm parameters, I see!Changed the parameters for MuonHoughSeeder, you have. From
inSimHitsandinDriftCirclestoinTruthSegments,inSpacePoints, andoutHoughMax, the transition is. Align with the refactoring of drift circles to space points and simulation hits to segments, this does. Added new output for Hough maxima, you have also. Good changes, these are!Examples/Scripts/Python/muon_hough.py (5)
7-7: Updated imports, yes!Removed old readers for drift circles and simHits, you have. Added new readers for space points and segments instead. Wise decision this is, aligning with our refactoring goals.
27-30: SpacePoint reader replaces drift circle reader!Use
CsvMuonSpacePointReadernow we do, with proper configuration for space points. Old drift circles, needed no more they are. Proper naming conventions you follow -outputSpacePoints="MuonSpacePoints". Good this is!
32-36: Added segment reader for truth information!New
CsvMuonSegmentReaderyou create, to read truth segments from CSV files. Replace old simHit reader, this does. Proper naming of output collection as"MuonTruthSegments", I observe. Maintain consistency throughout your changes, you do!
40-40: Added truth reader to sequencer!Important step forgotten, you have not. Added the new reader to the sequencer, ensuring it runs during execution.
42-46: Updated MuonHoughSeeder parameters!Configured seeder with new input and output collections, you have. Point to space points and truth segments now it does, instead of drift circles and simHits. Added output for Hough maxima with name "MuonHoughSeeds". Complete the refactoring chain, these changes do!
Examples/Framework/include/ActsExamples/EventData/MuonHoughMaximum.hpp (7)
1-12: Good header setup and dependencies!Proper copyright headers and include guard, you have added. Include only what needed is - the MuonSpacePoint header. Clean and focused, your code is!
13-19: Well-documented class description!Explained the purpose of this EDM class clearly, you have. Store results from Hough Transform, it does. Document both precision and non-bending direction parameters, your comments do. Make it easy to understand the purpose, this documentation does!
20-30: First constructor well implemented!Define a type alias for hit collection using MuonSpacePoint pointers, you do. First constructor for storing parameters from single Hough transform, well documented it is. Takes tanBeta, interceptY and associated hits. Initialize member variables properly in initializer list, you do!
31-44: Second constructor well implemented!Constructor for complementary Hough transform parameters, comprehensive it is. Handle both directions - precision and non-bending, it does. Properly document parameters, you do. Initialize all member variables in initializer list, good practice this is!
45-54: Accessor methods clear and const-correct!Provided methods to access all parameters and hits, you have. Made them const-correct, wise you are. Return by value for primitives and const reference for vectors, efficient this is!
55-61: Private member variables properly defined!Keep data members private, good encapsulation this is. Initialize with default values where appropriate. Store hits as vector of pointers, not objects themselves - efficient this is for large collections.
63-64: Container type alias provided!Define
MuonHoughMaxContaineras vector of MuonHoughMaximum objects. Make downstream code simpler, this will. Easy to understand and use in algorithms, this container is.Examples/Io/Csv/src/CsvMuonSegmentReader.cpp (2)
25-31: Hmmm, robust input checks you have.
Effective these validations are. Correctly, the code ensures the presence of input stem and output segment.
66-70: Verify bit shifts, you must.
Shifting bits to extract station, side, and sector, the code is. Triple-check correctness to avoid mismatch in station indexing.Examples/Io/Csv/src/CsvOutputData.hpp (2)
69-100: Clear and thorough, these structs look.
A strong representation of muon segment data, they provide. Additional safety or range checks, consider you might.
109-147: Complete your space point data structure is.
Well-labeled each field is. For clarity, any future alignment with geometry transformations, keep in mind you should.Examples/Io/Csv/src/CsvMuonSpacePointReader.cpp (3)
28-36: Yes, verifying loaded configuration, done well is.
Empty input or missing output, you watch for. Proper exceptions you raise. Approved, it is.
59-76: Bucket logic, most cunning it is.
Start a new bucket if station or bucket differs, you do. A healthy approach, yes, but confirm transitions you must.
83-90: Coordinate definitions and normal, consistent they appear.
Precise transformations they reflect. The drift radius usage also correct it seems. Good code, this is.Examples/Python/src/Input.cpp (3)
14-15: Needed, these includes are. Good addition, they seem.
All references toCsvMuonSegmentReaderandCsvMuonSpacePointReaderbecome clear, yes. No conflict do I foresee.
78-80: Added, new CSV reader for muon segments. Excellent, it is.
Verify you have all configuration fields properly set in the Python code or downstream classes for consistent operation, you must.
81-83: Introduced,CsvMuonSpacePointReaderhas been.
Check usage in top-level scripts, ensure correct filename stems and output handles, you should.Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvMuonSegmentReader.hpp (4)
25-35: Update documentation, consistent it is with muon segments now.
A clear reference to CSV segment input, good. Mismatch with old drift circle references, none I see.
43-43: Renamed output handle, well done this is.
Clarity you have improved, reflecting the new data structure.
50-50: Constructing, the segment reader does.
No issues appear. Parameters consistent, they are.
68-69: Write handle updated toMuonSegmentContainer.
Implementation aligned with the constructor, it is. Properly verified, ensure usage is.Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvMuonSpacePointReader.hpp (2)
32-36: Default initialization you have introduced, good practice it is.
Memory safety and clarity improved, consistent usage confirm.Also applies to: 57-59
61-62: Write handle toMuonSpacePointContainer, well done.
Check that the "OutputMuonSpacePoints" label is used consistently across the code, you must.Examples/Algorithms/TrackFinding/src/MuonHoughSeeder.cpp (4)
11-14: Additional includes declared, progress indicated.
Changes reveal broader refactoring for muon data, correct they seem.Also applies to: 18-18, 24-25, 29-30
34-49: New lambdas for drift-circle-like and strip parameterization, introduced you have.
Though referencing ‘DC’ inMuonSpacePointcode, clarity that these are conceptual solutions for space points, verify. Consistency check recommended, it is.Also applies to: 51-57, 60-65
67-84: Constructor refactored, verifyinginSpacePointsandinTruthSegmentsyou are.
Exception thrown if missing, well done. Good stability practice, mmh.
324-333: Initialize/finalize methods for drawing Hough histograms, a good approach you have.
Potential for modular logging or lazy initialization, there is. But critical problem, see I do not.Also applies to: 334-337
Examples/Framework/src/EventData/MuonSpacePoint.cpp (4)
1-28: Elegant output stream implementation, I sense.For both MuonId and MuonSpacePoint objects, well-formatted display logic you have created. Good use of std::format and Acts::toString, making debugging easier, it will.
31-96: Comprehensive string conversions for enumerations, you have provided.For StationName, TechField, and DetSide enums, thorough string representation functions you have written. Default "Unknown" case handled wisely, preventing unexpected behavior when unknown enum values appear.
97-111: Simple and effective setter methods for MuonId, I observe.Clear implementation of modifier functions that update chamber properties, layer/channel information, and coordinate flags. The purpose of each parameter, well-documented it is.
112-135: Efficient move semantics for coordinate definitions, wise choice this is.Using std::move for vector parameters in defineCoordinates and defineNormal, avoid unnecessary copies you do. Other setter methods, simple and focused they remain.
Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/MuonHoughSeeder.hpp (3)
11-22: Updated includes to support new data structures, I see.Replaced old dependencies with MuonHoughMaximum, MuonSegment, and MuonSpacePoint headers. For the refactoring from drift circles to space points, essential these changes are.
47-55: Configuration structure updated for new data model, good this is.Replaced old string members with new inTruthSegments, inSpacePoints, and outHoughMax. Added margin parameters for eta and phi Hough planes with sensible defaults of 10cm. This improves configurability of algorithm, hmm.
76-80: Data handling updated to use new containers, aligned with PR objectives this is.ReadDataHandles now reference MuonSegmentContainer and MuonSpacePointContainer instead of SimHitContainer and DriftCircleContainer. New WriteDataHandle for MuonHoughMaxContainer added. These changes fulfill the PR goal of replacing drift circles with space points, yes.
Examples/Framework/include/ActsExamples/EventData/MuonSpacePoint.hpp (4)
20-24: Well-documented class purpose, I approve.Clear explanation of the space point concept and its coordinate system orientation relative to ATLAS Monitored Drift Tubes. Helps readers understand the class design intent, this does.
27-116: Comprehensive MuonId helper class with clear enumerations, well-structured it is.For measurement identification, well-designed enumerations (TechField, StationName, DetSide) with good accessor and modifier methods you have created. The sameStation method provides useful functionality for comparing stations.
118-161: Core MuonSpacePoint methods and data members, well-designed they are.Good balance of accessor and mutator methods, with clear parameter naming and documentation. Default initialization for member variables, proper it is. The setSpatialCov method provides direct access to specific covariance matrix elements.
162-182: Container definition and helper functions, complete the interface they do.MuonSpacePointContainer as jagged vector with clear documentation, and proper to_string declarations for all enumeration types. Stream operators for both MuonId and MuonSpacePoint, debugging they will aid.
Examples/Framework/include/ActsExamples/EventData/MuonSegment.hpp (4)
19-29: Class definition with standard constructors, well-established it is.Appropriately reuses MuonId from MuonSpacePoint, avoiding duplication. Default, copy, and move operations provided with sensible defaults.
31-52: Comprehensive accessor methods, clear and concise they are.All important segment properties accessible through const methods: positions and directions in both global and local coordinates, identifier, time information, quality metrics, and hit counts.
53-81: Setter methods with logical parameter grouping, intuitive API they create.Methods grouped by related functionality: coordinate setting, ID assignment, time properties, fit quality, and hit statistics. Clear parameter naming makes usage straightforward, it does.
83-102: Private members with proper initialization, robust they make the class.All numeric fields initialized to sensible defaults (zero) and vectors to Zero(). Good documentation for member variables explaining their purpose, especially for the different hit counters.
Examples/Framework/include/ActsExamples/EventData/MuonSpacePoint.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Examples/Framework/include/ActsExamples/EventData/MuonSpacePoint.hpp (1)
26-116: Well-structured MuonId helper class, this is!Comprehensive identifier system for muon measurements, this nested class provides. Technology types, station names, and detector sides, all encoded they are. Methods to set and retrieve properties, included they are. Station comparison functionality, useful it will be for chamber-based processing.
Fix C-style types, you must:
- uint8_t sector() const { return m_sector; } + std::uint8_t sector() const { return m_sector; } - uint8_t detLayer() const { return m_layer; } + std::uint8_t detLayer() const { return m_layer; } - uint16_t channel() const { return m_channel; } + std::uint16_t channel() const { return m_channel; }🧰 Tools
🪛 GitHub Check: lint
[failure] 80-80: Do not use C-style uint16_t
Replace uint16_t with std::uint16_t
[failure] 78-78: Do not use C-style uint8_t
Replace uint8_t with std::uint8_t
[failure] 74-74: Do not use C-style uint8_t
Replace uint8_t with std::uint8_tExamples/Io/Csv/src/CsvMuonSpacePointReader.cpp (1)
40-42: Redundant namespace qualifier, remove you should!Duplicated class name in method declaration, unnecessary this is.
-std::string CsvMuonSpacePointReader::CsvMuonSpacePointReader::name() const { +std::string CsvMuonSpacePointReader::name() const { return "CsvMuonSpacePointReader"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Examples/Algorithms/TrackFinding/src/MuonHoughSeeder.cpp(2 hunks)Examples/Framework/include/ActsExamples/EventData/MuonSpacePoint.hpp(1 hunks)Examples/Io/Csv/src/CsvMuonSpacePointReader.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Examples/Algorithms/TrackFinding/src/MuonHoughSeeder.cpp
🧰 Additional context used
🧬 Code Definitions (1)
Examples/Framework/include/ActsExamples/EventData/MuonSpacePoint.hpp (2)
Examples/Framework/include/ActsExamples/EventData/MuonSegment.hpp (8)
m_id(40-40)m_time(42-42)id(64-64)id(64-64)pos(54-57)pos(54-54)pos(59-62)pos(59-59)Examples/Framework/src/EventData/MuonSpacePoint.cpp (4)
ostr(15-21)ostr(15-16)ostr(22-28)ostr(22-23)
🪛 GitHub Check: lint
Examples/Framework/include/ActsExamples/EventData/MuonSpacePoint.hpp
[failure] 80-80: Do not use C-style uint16_t
Replace uint16_t with std::uint16_t
[failure] 78-78: Do not use C-style uint8_t
Replace uint8_t with std::uint8_t
[failure] 74-74: Do not use C-style uint8_t
Replace uint8_t with std::uint8_t
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: merge-sentinel
- GitHub Check: build_debug
🔇 Additional comments (13)
Examples/Framework/include/ActsExamples/EventData/MuonSpacePoint.hpp (7)
19-24: New MuonSpacePoint class introduced, hmm!A new Event Data Model for muon space points, this class provides. Replace the old drift circle EDM, it does. In local frame expressed, these space points are, with x-axis parallel to MDT tubes, yes.
124-136: Accessor methods for space point properties, well defined they are!Complete set of getter methods, this class provides. Position, direction, normal vector, covariance, drift radius and time - all essential properties for muon track reconstruction, these are.
Return wrong member variable in stripPlaneNormal method, I sense not. Fixed in previous commit, this issue was.
138-152: Setter methods for space point properties, properly declared they are!Methods to define identifier, coordinates, normal vector, radius, time and covariance - complete API for configuring space point properties, this provides. Use of rvalue references for position vectors, efficient implementation this suggests.
153-161: Private member variables, well initialized they are!Default initialization for all member variables, prevent undefined behavior this does. Zero vectors and identity matrix for covariance, sensible defaults these are.
162-165: Jagged vector container for space points, interesting choice this is!Organize space points in buckets, this container structure does. For efficient spatial proximity queries, useful this can be. Good comment explaining the organization, clarity it brings.
167-172: String conversion functions for enum types, helpful these are!For debugging and logging purposes, essential these functions are. Convert enum values to readable strings, they do. Well-named and clearly documented, they are.
177-182: Stream operators for printing, useful they are!For debugging and logging, essential these operators are. Consistent with other EDM classes in the codebase, this implementation is. Global namespace, these functions properly occupy.
Examples/Io/Csv/src/CsvMuonSpacePointReader.cpp (6)
23-38: Constructor validates configuration, good practice this is!Check for empty input filename stem and output collection, prevents runtime errors this does. Initialize event range and output writer properly, constructor does. Exception with clear message, thrown for invalid configuration it is.
44-47: Simple accessor method for events range, good this is!Provide access to private member, this method does. No unnecessary computation, efficient it is.
49-101: Main reading logic, well-structured it is!Step by step, CSV file processing this method performs:
- Path to input file, correctly constructed it is
- CSV reader for MuonSpacePointData, initialized it is
- Space point container, created it is
- Fields from data record, properly decoded they are
- Space points into buckets, organized they are
- Properties of each space point, completely defined they are
- Output to event store, properly handled it is
Organization by buckets based on sector ID and bucket ID, efficient processing this enables. Bitwise operations for decoding identifiers, clear and effective they are.
59-60: Alias and initial bucket tracking, readability they improve!Using alias for MuonId, reduce code verbosity this does. Track last bucket ID to detect changes, essential for organizing space points this is.
61-94: Space point creation and property setup, thorough this is!From CSV data to MuonSpacePoint objects, complete conversion this implementation provides. Bitwise decoding of identifier fields, properly handled it is. Spatial properties and covariance, completely defined they are.
Verbose logging of new space points, helpful for debugging this will be.
97-100: Writing to output and returning success code, complete the processing this does!Move semantics for transferring space points to output, efficient this is. Clear success indication returned, proper error handling this enables.
junggjo9
changed the title
junggjo9
changed the title
📊: Physics performance monitoring for d2b4be9Full contents |
junggjo9
changed the title
junggjo9
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
Examples/Framework/include/ActsExamples/EventData/MuonSegment.hpp (1)
65-66: Typo in doc string, fix we must.
“unceratinty,” spelled it is. “uncertainty,” correct it should be.- /** @brief Set the time & unceratinty */ + /** @brief Set the time & uncertainty */Examples/Io/Csv/src/CsvMuonSpacePointReader.cpp (2)
19-19: Unused include, suspicious it is.
No references to "CsvOutputData.hpp" found, I see. Remove or justify its presence, you must.-#include "CsvOutputData.hpp"
47-51: Additional CSV error handling, consider you should.
If the file is missing or columns mismatch, gracefully handle these cases, the code does not. Improving robustness, it will.Examples/Algorithms/TrackFinding/src/MuonHoughSeeder.cpp (1)
70-72: Change drift circle reference in error message, you must.
Space points, the code now expects, but old wording “drift circle collection” remains. Correct alignment with reality, needed it is.- throw std::invalid_argument("MuonHoughSeeder: Missing drift circle collection"); + throw std::invalid_argument("MuonHoughSeeder: Missing space point collection");Examples/Framework/src/EventData/MuonSpacePoint.cpp (1)
1-16: License header repeated, it is.
Duplicate license block from lines 1-7 and 10-16, I see. Remove or merge them, you should.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Examples/Algorithms/TrackFinding/src/MuonHoughSeeder.cpp(2 hunks)Examples/Framework/include/ActsExamples/EventData/MuonHoughMaximum.hpp(1 hunks)Examples/Framework/include/ActsExamples/EventData/MuonSegment.hpp(1 hunks)Examples/Framework/include/ActsExamples/EventData/MuonSpacePoint.hpp(1 hunks)Examples/Framework/src/EventData/MuonSpacePoint.cpp(1 hunks)Examples/Io/Csv/src/CsvMuonSpacePointReader.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Examples/Framework/include/ActsExamples/EventData/MuonHoughMaximum.hpp
🧰 Additional context used
🧬 Code Definitions (1)
Examples/Framework/include/ActsExamples/EventData/MuonSpacePoint.hpp (2)
Examples/Framework/include/ActsExamples/EventData/MuonSegment.hpp (8)
m_id(40-40)m_time(42-42)id(64-64)id(64-64)pos(54-57)pos(54-54)pos(59-62)pos(59-59)Examples/Framework/src/EventData/MuonSpacePoint.cpp (4)
ostr(23-29)ostr(23-24)ostr(30-36)ostr(30-31)
⏰ Context from checks skipped due to timeout of 90000ms (22)
- GitHub Check: CI Bridge / lcg_107: [alma9, clang19]
- GitHub Check: CI Bridge / lcg_107: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_107: [alma9, gcc13]
- 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_2404_clang19
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / build_gnn_tensorrt
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: macos
- GitHub Check: linux_ubuntu
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu_extra (ubuntu2404_clang19, 20, clang++-19)
- GitHub Check: unused_files
- GitHub Check: missing_includes
- GitHub Check: build_debug
🔇 Additional comments (3)
Examples/Framework/include/ActsExamples/EventData/MuonSegment.hpp (2)
1-7: Preserve the license block, you must.
Looks properly included, hmmm. Approved, it is.
19-103: Strong in clarity and default construction, your class is.
Minimal overhead, yes. Well done, your defaulted methods are.Examples/Framework/include/ActsExamples/EventData/MuonSpacePoint.hpp (1)
98-111:❓ Verification inconclusive
Approve the code, I do, but sector overflow, watch you must.
Asectoras int is stored into astd::uint8_t. Ensure the range is valid, you should, or add checks if large values possible.
🏁 Script executed:
#!/bin/bash # Searching for setChamber calls, verifying sector usage is < 256 rg --context 3 "setChamber"Length of output: 2882
Sector Value Range – Caution, You Must
Approved the code, I do. Verified that all current call sites mask the sector using 0xFF, they are. However, note well: the sector parameter, passed as an int and stored as a std::uint8_t, remains vulnerable if future callers do not mask appropriately. Consider adding an explicit range check or assert in setChamber for clarity and protection, you should.
- In Examples/Framework/include/ActsExamples/EventData/MuonSpacePoint.hpp (lines 98–111), the sector field is stored as a std::uint8_t.
- Investigation via rg confirms that in CsvMuonSpacePointReader.cpp and CsvMuonSegmentReader.cpp the sector is computed with 0xFF masking.
- Futureproof your code, it will, if an internal check, you add there.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Examples/Framework/src/EventData/MuonSpacePoint.cpp (1)
9-15: Duplicated license header lines, I sense.
Reduced duplication, keep code tidy we should.Examples/Framework/include/ActsExamples/EventData/MuonSpacePoint.hpp (1)
178-180: Typo in the comment, "osteram" it says.
Correct word "ostream" it should read, fix the doc comment.-/// @brief osteram operator of the Space point +/// @brief ostream operator of the Space point
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Examples/Framework/include/ActsExamples/EventData/MuonHoughMaximum.hpp(1 hunks)Examples/Framework/include/ActsExamples/EventData/MuonSpacePoint.hpp(1 hunks)Examples/Framework/src/EventData/MuonSpacePoint.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Examples/Framework/include/ActsExamples/EventData/MuonHoughMaximum.hpp
🧰 Additional context used
🧬 Code Definitions (1)
Examples/Framework/include/ActsExamples/EventData/MuonSpacePoint.hpp (2)
Examples/Framework/include/ActsExamples/EventData/MuonSegment.hpp (8)
m_id(40-40)m_time(42-42)id(64-64)id(64-64)pos(54-57)pos(54-54)pos(59-62)pos(59-59)Examples/Framework/src/EventData/MuonSpacePoint.cpp (4)
ostr(22-28)ostr(22-23)ostr(29-35)ostr(29-30)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: CI Bridge / lcg_107: [alma9, clang19]
- GitHub Check: CI Bridge / lcg_107: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_107: [alma9, gcc13]
- 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_2404_clang19
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_gnn_tensorrt
- GitHub Check: linux_ubuntu_extra (ubuntu2404_clang19, 20, clang++-19)
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: macos
- GitHub Check: missing_includes
- GitHub Check: build_debug
🔇 Additional comments (10)
Examples/Framework/src/EventData/MuonSpacePoint.cpp (4)
20-21: Check compiler support forstd::format, you must.
C++20 required it is; ensure environment, do.
22-35: Operator overloads, well structured they are.
The formatting approach, readable and consistent it looks.
42-103: Enum conversions, helpful they are.
Clear strings you provide, good for debugging it is.
133-139: Covariance storage, double-check correctness we must.
Matrix indexes, ensure alignment with data we do.Examples/Framework/include/ActsExamples/EventData/MuonSpacePoint.hpp (6)
1-7: Good license inclusion, you have.
All major disclaimers, present they are.
18-23: Clear class overview, this is.
The documentation describing local coordinate frame, quite thorough it seems.
28-36: Strong typed enumerations, a wise choice.
Readability and safety, these enums provide.
89-91: Bravo on sameStation method.
Performs match checks for station, sector, and side, correct it is.
129-129: Fixed the stripPlaneNormal return, you have.
Previously returning m_pos, repeating that mistake you must not.
57-77:✅ Verification successful
Observe the side 'A' and 'C' naming conventions, you must.
Potential confusion or mismatch with official naming, verify you should.Use a script to confirm these constants align with the domain’s existing enum definitions:
🏁 Script executed:
#!/bin/bash # Searching for references to DetSide usage for consistency rg -A 5 $'DetSide::'Length of output: 1165
Naming conventions aligned, they are.
After verifying with RG output, the constants "A" and "C" in theDetSideenum are used consistently throughout the codebase. The "A-side" and "C-side" nomenclature in the MuonSpacePoint implementation confirms the official domain naming. No further modifications are required, continue you must.
|
I thought that format` is already C++20. and the image also looks like a c++ 20 one. |
AJPfleger
changed the title
…oduce HoughMaximum container and remove the MuonSimHit in favour ot the MuonSegment
junggjo9
force-pushed
the
IntroduceMuonSpacePoint
branch
from
7df0c1c to
16c7c5d
Compare
|
junggjo9
force-pushed
the
IntroduceMuonSpacePoint
branch
from
f57671a to
d2b4be9
Compare
…
For Phase-II muon, the drift circle EDM has been replaced by the more generic SpacePoint EDM. This MR replaces the muon drift circle by the more generic MuonSpace point.
Further, the MuonHoughAlgorithm is also refactored to estimate the track parameters in the non precision plane
Introduce the MuonHoughMaximum container to process the hough output in subsequent steps
MuonSimHits are replaced by the MuonSegments. These are the straight line parameter summaries of the individual truth hits in the chamber.
Introduce the boiler plate code to read the segments & space points from CSVs. The reference CSV file may be obtained in ATLAS athena using https://gitlab.cern.ch/atlas/athena/-/blob/main/MuonSpectrometer/MuonPhaseII/MuonCnv/MuonHitCsvDump/python/csvHitDump.py
Remove the unneeded DriftCircle / MuonSimHit code
--- END COMMIT MESSAGE ---
This MR is the first breakout of the big segment fit porting (#4108)
Tagging: @goblirsc, @dimitra97
Summary by CodeRabbit
New Features
Chores