refactor: build TimingDetector geometry in C++ instead of GeoModelXML#62
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR replaces GeoModelXML-based TimingDetector construction with direct C++ placement of 330 analytically positioned bars, adds the ChangesTimingDetector C++ Construction Migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TimingDetectorFactory
participant GeoModelKernel
participant TimingDetectorTest
TimingDetectorFactory->>GeoModelKernel: create container and reusable bar logical volume
loop 3 columns x 110 rows
TimingDetectorFactory->>GeoModelKernel: place named bar with computed transform
end
TimingDetectorFactory->>TimingDetectorTest: return built detector volume
TimingDetectorTest->>TimingDetectorTest: assert 330 bars and 330 child volumes
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/SHiPMaterials.cpp`:
- Around line 203-211: The TimDetScint material definition is internally
inconsistent because its name/comment indicate PVT scintillator while the
Carbon/Hydrogen fractions match Polystyrene. Update the material in
SHiPMaterials::SHiPMaterials so it is either renamed/commented to reflect the
polystyrene composition or changed to the intended PVT C9H10 fractions, and make
sure the density and element fractions agree with the chosen material
definition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd3a8034-3eda-49ed-bafa-eb1eeea66544
📒 Files selected for processing (8)
src/SHiPMaterials.cppsubsystems/TimingDetector/CMakeLists.txtsubsystems/TimingDetector/README.mdsubsystems/TimingDetector/data/timing_detector.gmxsubsystems/TimingDetector/include/TimingDetector/SHiPTimingDetInterface.hsubsystems/TimingDetector/include/TimingDetector/TimingDetectorFactory.hsubsystems/TimingDetector/src/TimingDetectorFactory.cppsubsystems/TimingDetector/test_timingdetector.cpp
💤 Files with no reviewable changes (2)
- subsystems/TimingDetector/include/TimingDetector/SHiPTimingDetInterface.h
- subsystems/TimingDetector/data/timing_detector.gmx
✅ prek hooks passed |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@subsystems/TimingDetector/README.md`:
- Around line 11-15: The fenced geometry example in the README needs a language
hint to satisfy markdownlint. Update the code block around the Timing_Detector
layout example to use an explicit fence tag such as text, keeping the content
unchanged; this is the only change needed in the README example block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 943c8c32-2226-43f8-b0c0-ec4fc36e513f
📒 Files selected for processing (8)
src/SHiPMaterials.cppsubsystems/TimingDetector/CMakeLists.txtsubsystems/TimingDetector/README.mdsubsystems/TimingDetector/data/timing_detector.gmxsubsystems/TimingDetector/include/TimingDetector/SHiPTimingDetInterface.hsubsystems/TimingDetector/include/TimingDetector/TimingDetectorFactory.hsubsystems/TimingDetector/src/TimingDetectorFactory.cppsubsystems/TimingDetector/test_timingdetector.cpp
💤 Files with no reviewable changes (2)
- subsystems/TimingDetector/data/timing_detector.gmx
- subsystems/TimingDetector/include/TimingDetector/SHiPTimingDetInterface.h
🚧 Files skipped from review as they are similar to previous changes (4)
- src/SHiPMaterials.cpp
- subsystems/TimingDetector/test_timingdetector.cpp
- subsystems/TimingDetector/include/TimingDetector/TimingDetectorFactory.h
- subsystems/TimingDetector/src/TimingDetectorFactory.cpp
|
@coderabbitai review |
✅ Action performedReview finished.
|
The TimingDetector was the only subsystem still built from a GeoModelXML (.gmx) file loaded at runtime via Gmx2Geo. Its geometry is a regular grid of identical bars whose positions are fully analytic, so the GMX indirection added a GeoModelXml link dependency, DTD-copying CMake plumbing and a runtime path-resolution step without any benefit over a plain C++ factory like every other subsystem. TimingDetectorFactory::build() now places the 330 bars (3 columns x 110 rows) directly in two nested loops with analytic positions (x = (col-1)*1300, y = -3220 + row*6440/109, z = (row%2)*12 + (col%2)*90), reproducing the previous placement exactly. Bars share one GeoLogVol and are placed as GeoPhysVol with hierarchical /SHiP/timing_detector/bar_<col>_<row> names; sensitivity is assigned downstream by name pattern, matching the UpstreamTagger convention. A dedicated TimDetScint material (C8H8, 1.023 g/cm3) is added to SHiPMaterials. The SHiPTimingDetInterface, the .gmx data file and all GMX-related CMake plumbing are removed. The public API and the world placement are unchanged.
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
The TimingDetector was the only subsystem still built from a GeoModelXML (
.gmx) file loaded at runtime viaGmx2Geo. Its geometry is a regular grid of identical bars whose positions are fully analytic, so the GMX indirection only added aGeoModelXmllink dependency, DTD-copying CMake plumbing and a runtime path-resolution step — with no benefit over a plain C++ factory like every other subsystem. This ports it to native C++.Changes
TimingDetectorFactory::build()places the 330 bars (3 columns × 110 rows) directly in two nested loops with analytic positions, reproducing the previous placement exactly:x = (col-1)·1300mmy = -3220 + row·6440/109mmz = (row%2)·12 + (col%2)·90mm (stagger levels 0/12/90/102)GeoLogVoland are placed asGeoPhysVolwith hierarchical/SHiP/timing_detector/bar_<col>_<row>names; sensitivity is assigned downstream by name pattern, matching theUpstreamTaggerconvention.TimDetScintmaterial (C8H8, ρ = 1.023 g/cm³) toSHiPMaterials.SHiPTimingDetInterface, thetiming_detector.gmxdata file, and all GMX-related CMake plumbing (GeoModelXml link, DTD discovery/copy, compile definitions, install rules).The public API (
TimingDetectorFactory(SHiPMaterials&),build(),barCount()) and the world placement inSHiPGeometry.cppare unchanged.Verification
pixi run build— clean; noGeoModelXmlreference in TimingDetector.pixi run test— 33/33 pass, includingTimingDetectorBarCount, the builder, consistency Z-overlap checks, and the fullOverlapCheck(bar positions are identical to the GMX, so overlap results are unchanged).pixi run gdml— exports cleanly; the GDML contains theTimDetScintmaterial, one shared bar logvol, and 330 bar placements at the correct staggered positions.Notes
Two pre-existing conditions are preserved faithfully (not introduced here): the outermost bars reach y = ±3250 mm (exactly the container half-Y), and the bars span z ∈ [−5, +107] mm within the ±250 mm container. Neither trips the overlap check.
Summary by CodeRabbit
/SHiP/timing_detector/bar_<col>_<row>naming, including the z-stagger placement rule.TimDetScintscintillator material for timing detector bars.