feat(ms)!: Add configurable iron-block muon shield (FairShip TRY_2026)#53
feat(ms)!: Add configurable iron-block muon shield (FairShip TRY_2026)#53matclim wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMuonShield now reads geometry from ChangesLegacy MuonShield subsystem config-driven refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SHiPGeometryBuilder
participant MuonShieldFactory
participant NeutrinoDetector
SHiPGeometryBuilder->>MuonShieldFactory: build configured shield
SHiPGeometryBuilder->>NeutrinoDetector: build detector
SHiPGeometryBuilder->>MuonShieldFactory: embed daughter detector
MuonShieldFactory-->>SHiPGeometryBuilder: shield volume and centreZ_mm()
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (19)
stage/tests/test_consistency.cpp-13-16 (1)
13-16: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
<cmath>forstd::abs(double).tests/test_consistency.cppcallsstd::abson adoublebut doesn’t include<cmath>, so this depends on transitive includes from other headers.🤖 Prompt for 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. In `@stage/tests/test_consistency.cpp` around lines 13 - 16, tests/test_consistency.cpp uses std::abs on a double without including the proper header, so add the missing <cmath> include alongside the existing includes to make the dependency explicit and avoid relying on transitive headers. Update the include list near the top of the file where std::abs is used in the consistency test code.stage/tests/test_consistency.cpp-48-69 (1)
48-69: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the unreachable cavern skip.
/SHiP/cavernnever appears amongworld’s children here, so this branch is dead; if the cavern should be excluded fromExpectedSubsystemCount, the test needs to match the actual child being iterated.🤖 Prompt for 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. In `@stage/tests/test_consistency.cpp` around lines 48 - 69, The cavern filter in collectSubsystems is dead code because the loop over world children never encounters "/SHiP/cavern"; remove that unreachable skip, or if ExpectedSubsystemCount is meant to exclude the cavern then update collectSubsystems to iterate the actual child that represents it and filter that symbol instead. Keep the logic anchored in collectSubsystems, getChildVol, and getLogVol so the test matches the real world-child ordering.subsystems/MuonShield/src/MuonShieldConfig.cpp-36-40 (1)
36-40: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun clang-format to fix CI lint failure.
Pipeline logs report clang-format reformatting the
kKnownKeysarray spacing/alignment; current formatting fails the lint job.🤖 Prompt for 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. In `@subsystems/MuonShield/src/MuonShieldConfig.cpp` around lines 36 - 40, The kKnownKeys array formatting in MuonShieldConfig needs to be normalized to match clang-format, as the current spacing/alignment is causing the lint failure. Update the declaration in MuonShieldConfig.cpp by applying the project’s standard formatting to the static constexpr std::array kKnownKeys initializer so it matches clang-format output exactly.Source: Pipeline failures
subsystems/MuonShield/src/MuonShieldConfig.cpp-120-139 (1)
120-139: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun clang-format to fix CI lint failure.
Pipeline logs report clang-format reformatting the
block_width_x_mm/block_height_y_mmstd::runtime_errorthrow messages (wrapping/indentation); current formatting fails the lint job.🔧 Suggested fix
clang-format -i subsystems/MuonShield/src/MuonShieldConfig.cpp🤖 Prompt for 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. In `@subsystems/MuonShield/src/MuonShieldConfig.cpp` around lines 120 - 139, Run clang-format on MuonShieldConfig.cpp to match the repository style and fix the CI lint failure. The affected code is the validation block in MuonShieldConfigConfig parsing where the std::runtime_error messages for block_width_x_mm, block_height_y_mm, envelope_half_x_mm, and envelope_half_y_mm are wrapped differently than expected; reformat that block so the throw statements and concatenated strings follow clang-format output consistently.Source: Pipeline failures
subsystems/MuonShield/include/MuonShield/MuonShieldConfig.h-41-49 (1)
41-49: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun clang-format to fix CI lint failure.
Pipeline logs report clang-format reformatting
envelopeLengthZ_mm()from a multi-line body to a single line here; the committed formatting doesn't match the project's clang-format style and is failing theprek/lint CI job.🔧 Suggested fix
clang-format -i subsystems/MuonShield/include/MuonShield/MuonShieldConfig.h🤖 Prompt for 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. In `@subsystems/MuonShield/include/MuonShield/MuonShieldConfig.h` around lines 41 - 49, The MuonShieldConfig helper formatting does not match the project’s clang-format style, causing the lint CI failure. Update the formatting in MuonShieldConfig so the `envelopeLengthZ_mm()`, `envelopeCentreZ_mm()`, and `blockThicknessZ_mm()` accessors match clang-format’s preferred single-line/multi-line layout, using the existing `MuonShieldConfig` class definitions as the reference point, then run clang-format on the header to keep the committed style consistent.Source: Pipeline failures
stage/README.md-41-43 (1)
41-43: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocumented compiler requirement conflicts with actual CMake config.
README states "C++20 compatible compiler" as a prerequisite, but
stage/CMakeLists.txtsetsCMAKE_CXX_STANDARD 23(required by Phlex per its comment). Update the README to match.🤖 Prompt for 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. In `@stage/README.md` around lines 41 - 43, The README prerequisite list is out of sync with the build configuration because it says “C++20 compatible compiler” while the CMake setup in stage/CMakeLists.txt requires C++23 for Phlex. Update the compiler requirement text in stage/README.md to match the actual CMAKE_CXX_STANDARD setting and keep the rest of the prerequisite list unchanged.stage/REUSE.toml-4-4 (1)
4-4: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDownload location URL inconsistent with README badge.
This file lists
SPDX-PackageDownloadLocation = "https://github.com/SHiP-Collaboration/geometry", whilestage/README.md's badge links tohttps://github.com/ShipSoft/Geometry. Confirm which org/repo path is canonical and align both.🤖 Prompt for 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. In `@stage/REUSE.toml` at line 4, The SPDX-PackageDownloadLocation in REUSE.toml is inconsistent with the repository URL used in the stage/README.md badge. Confirm the canonical org/repo path and update the SPDX-PackageDownloadLocation value to match it, keeping the README badge and the REUSE.toml metadata aligned. Use the existing SPDX-PackageDownloadLocation symbol in REUSE.toml and the badge target in stage/README.md to verify both point to the same canonical location.stage/subsystems/NeutrinoDetector/README.md-73-79 (1)
73-79: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocumented test name doesn't match the actual test case.
The README lists
NeutrinoDetectorIsBoxContainer, but the referenced test file defines the case asTEST_CASE("NeutrinoDetectorWithinEnvelope", ...).Proposed fix
-- `NeutrinoDetectorIsBoxContainer` - the container is a `GeoBox` large enough to +- `NeutrinoDetectorWithinEnvelope` - the container is a `GeoBox` large enough to hold the contents.🤖 Prompt for 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. In `@stage/subsystems/NeutrinoDetector/README.md` around lines 73 - 79, The README references the wrong test case name for the container check: it says NeutrinoDetectorIsBoxContainer, but the actual test in test_neutrinodetector.cpp is NeutrinoDetectorWithinEnvelope. Update the documentation entry in the NeutrinoDetector README to use the exact test name from the TEST_CASE definition, and keep the other listed test names unchanged.stage/subsystems/NeutrinoDetector/README.md-38-38 (1)
38-38: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winGeometry-tree box dimensions don't match the factory constants.
The container is documented as
640 x 640 x 4100 mm, butNeutrinoDetectorFactory.hdefiness_halfX = 400.0,s_halfY = 400.0,s_halfZ = 2550.0(full box800 x 800 x 5100 mm), and the test intest_neutrinodetector.cppchecks against these same 400/400/2550 half-length bounds. The documented dimensions appear stale relative to the current header.Proposed fix
-/SHiP/neutrino_detector (Air, 640 x 640 x 4100 mm box) +/SHiP/neutrino_detector (Air, 800 x 800 x 5100 mm box)🤖 Prompt for 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. In `@stage/subsystems/NeutrinoDetector/README.md` at line 38, The NeutrinoDetector container dimensions in the README are stale and should match the current geometry constants used by NeutrinoDetectorFactory and the tests. Update the documented box size to align with the half-lengths defined in NeutrinoDetectorFactory.h (and the expectations in test_neutrinodetector.cpp), or explicitly note that the README was outdated if the factory values are the source of truth. Keep the geometry description consistent with the symbols s_halfX, s_halfY, and s_halfZ so the docs reflect the actual detector bounds.stage/subsystems/Trackers/src/TrackersFactory.cpp-253-280 (1)
253-280: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStraw gas
GeoNameTagisn't unique per straw.Every straw's gas daughter is tagged with the literal
"/SHiP/trackers/straw_gas"(Line 274), unlike the log-volume names (straw_wall_<uid>,straw_gas_<uid>) which correctly embeduidfor uniqueness. With 9600 straws, all gas children under different wall parents share the identical NameTag string, so any downstream code that resolves volumes via cumulative NameTag/IdentifierTag path (rather than log-volume name) cannot distinguish individual straws' gas volumes.Proposed fix
- wallPhys->add(new GeoNameTag("/SHiP/trackers/straw_gas")); + wallPhys->add(new GeoNameTag("/SHiP/trackers/straw_gas_" + std::to_string(uid)));🤖 Prompt for 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. In `@stage/subsystems/Trackers/src/TrackersFactory.cpp` around lines 253 - 280, The straw gas child in TrackersFactory::buildStraw is using a shared GeoNameTag string, so each straw’s gas volume is indistinguishable from the others. Update the GeoNameTag for the gas daughter to include the uid, matching the unique naming pattern already used for the GeoLogVol objects in buildStraw (straw_wall_<uid> and straw_gas_<uid>). Keep the rest of the wall/gas construction and GeoIdentifierTag flow unchanged.stage/subsystems/Target/README.md-13-13 (1)
13-13: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd language to fenced code block.
Static analysis flags this fence as missing a language identifier.
📝 Proposed fix
-``` +```text target_vacuum_box (Vacuum, 160×227.1×300 cm)🤖 Prompt for 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. In `@stage/subsystems/Target/README.md` at line 13, The fenced code block in the README for the vacuum box entry is missing a language identifier, which triggers static analysis. Update the Markdown fence for the target_vacuum_box snippet to use a text-language fenced block, keeping the existing content unchanged, so the block is explicitly labeled in the README.Source: Linters/SAST tools
stage/subsystems/MuonShield/README.md-19-19 (1)
19-19: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd language to fenced code block.
Static analysis flags this fence as missing a language identifier.
📝 Proposed fix
-``` +```text muon_shield (Air envelope, 4200 × 4600 × 27480 mm) centre z = 18.48 m🤖 Prompt for 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. In `@stage/subsystems/MuonShield/README.md` at line 19, The fenced block in the MuonShield README is missing a language tag, which triggers the static analysis warning. Update the Markdown fence around the muon_shield description to use an explicit language identifier, using the fenced code block in the README so the rendered content stays the same while satisfying linting.Source: Linters/SAST tools
stage/subsystems/MuonShield/src/MuonShieldConfig.cpp-34-40 (1)
34-40: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winclang-format check failing on
kKnownKeysarray.CI reports clang-format would reformat this array's spacing/alignment.
🤖 Prompt for 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. In `@stage/subsystems/MuonShield/src/MuonShieldConfig.cpp` around lines 34 - 40, The kKnownKeys initializer in MuonShieldConfig should be reformatted to match clang-format’s preferred spacing and alignment. Update the std::array declaration so the string literals are laid out in the formatter’s canonical style, keeping the kKnownKeys symbol and its sorted key list intact while removing manual alignment that causes CI diffs.Source: Pipeline failures
stage/subsystems/MuonShield/src/MuonShieldConfig.cpp-120-140 (1)
120-140: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winclang-format check failing on validation
throwmessages.CI reports clang-format would reformat the wrapped
std::runtime_errorstring concatenation/indentation for theblock_width_x_mm/block_height_y_mmchecks. Run clang-format locally to match CI expectations before merge.🤖 Prompt for 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. In `@stage/subsystems/MuonShield/src/MuonShieldConfig.cpp` around lines 120 - 140, The validation throws in MuonShieldConfig are formatted in a way that clang-format will rewrap, so the current `std::runtime_error` string concatenation/indentation for the block and envelope size checks needs to match the formatter’s style. Update the exception message construction in `MuonShieldConfig` so the `block_width_x_mm`, `block_height_y_mm`, `envelope_half_x_mm`, and `envelope_half_y_mm` checks follow the same wrapping/indentation pattern clang-format expects, then run clang-format on the file and keep the resulting formatting.Source: Pipeline failures
stage/subsystems/Calorimeter/README.md-25-25 (1)
25-25: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winZ placement value conflicts with
CalorimeterFactory.h.This README states z = 96970 mm, matching
calo.toml'sdetector_offset_z_mm, butCalorimeterFactory.h's class doc says the container is "centred at Z = 98 320 mm". One of the two is stale.🤖 Prompt for 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. In `@stage/subsystems/Calorimeter/README.md` at line 25, The Calorimeter placement documentation is inconsistent between the README and CalorimeterFactory.h. Verify the correct Z position used by the calorimeter container, then update the stale docstring in CalorimeterFactory and/or the README so both reference the same value as detector_offset_z_mm in calo.toml. Make the wording consistent around the container being centered at Z and ensure the single source of truth is reflected in the relevant comments.stage/subsystems/Calorimeter/src/CaloFibreHPLayer.cpp-61-70 (1)
61-70: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSublayers 0 and 2 collapse into duplicate overlapping fibres when
casingZ ≤ 2×rOuter.
dz = std::max(0.0, 0.5 * casingZ - rOuter)becomes0wheneverhpl_thickness_mmis small relative to the fibre diameter. Withdz == 0, sublayers 0 and 2 share identicaldx(both0.0) andzLocal(-0 == 0 == +0), so they place fibres at exactly the same positions — perfectly overlapping duplicate physical volumes rather than the intended 3-sublayer stagger. This doesn't trigger with the current defaultcalo.tomlvalues, but nothing prevents a future config from hitting it.🔧 Proposed guard
- const double dz = std::max(0.0, 0.5 * casingZ - rOuter); + const double dz = 0.5 * casingZ - rOuter; + if (dz <= 0.0) + throw std::runtime_error( + "CaloFibreHP::buildLayer: casing Z is too thin to stagger 3 fibre sublayers " + "(hpl_thickness_mm must exceed roughly 2x fiber_diameter_mm)");🤖 Prompt for 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. In `@stage/subsystems/Calorimeter/src/CaloFibreHPLayer.cpp` around lines 61 - 70, In CaloFibreHPLayer’s fibre tiling setup, the three-sublayer layout collapses when dz becomes zero, causing sublayers 0 and 2 to generate duplicate overlapping fibres. Update the sublayer placement logic so the staggered pattern is only used when casingZ is large enough, or otherwise fall back to a reduced layout/adjusted offsets that guarantee each sublayer has distinct x/z positions. Use the existing symbols pitch, dx, dz, zLocal, and the tiling block in CaloFibreHPLayer to apply the guard cleanly.stage/subsystems/Calorimeter/src/CalorimeterConfig.cpp-99-123 (1)
99-123: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winLegacy comma-string parsing can throw an undocumented exception type.
std::stoi(token.substr(...))in the legacy string branch isn't wrapped in a try/catch. A malformed token (e.g. non-numeric text, or a token that's just;/whitespace-adjacent junk left after trimming) will letstd::invalid_argument/std::out_of_rangeescape uncaught, whereas the documented contract forreadCaloConfigonly promisesstd::runtime_errorfor malformed input. Callers that catchstd::runtime_errorspecifically (per the API contract) won't catch this.🔧 Proposed fix
if (first == std::string::npos) continue; - out.push_back(std::stoi(token.substr(first, last - first + 1))); + try { + out.push_back(std::stoi(token.substr(first, last - first + 1))); + } catch (const std::exception&) { + throw std::runtime_error("CalorimeterConfig: '" + key + + "' contains a non-integer token: '" + token + "'"); + }🤖 Prompt for 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. In `@stage/subsystems/Calorimeter/src/CalorimeterConfig.cpp` around lines 99 - 123, The legacy comma-string parsing in readIntList can leak std::invalid_argument or std::out_of_range from std::stoi, which breaks the expected error contract for readCaloConfig. Update the string branch in CalorimeterConfig::readIntList to catch parsing failures around the token conversion and rethrow them as std::runtime_error with a clear CalorimeterConfig/key-specific message, so malformed legacy list values are handled consistently with the array branch.stage/subsystems/TimingDetector/data/timing_detector.gmx-26-27 (1)
26-27: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winOuter bar row touches the container boundary with zero margin.
The outermost bar rows are at Y=±3220.000mm; with the 30mm bar half-width (Line 42 in the header/shape def) this puts the bar face at exactly ±3250mm — precisely equal to
s_containerHalfY(3250.0, seeTimingDetectorFactory.hLine 39). This is a coincident-face condition. The siblingUpstreamTaggerFactory.cppexplicitly avoids this exact scenario vias_env_z_margin("coincident faces trigger GeomNav stuck-track warnings"), but no equivalent margin exists here. Since the container size is fixed by the envelope contract (test comment:halfY ≤ 3250), the fix should shrink the Y grid slightly (e.g., a few mm) rather than grow the container.🤖 Prompt for 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. In `@stage/subsystems/TimingDetector/data/timing_detector.gmx` around lines 26 - 27, The outer bar rows in the timing detector grid are exactly touching the container boundary, creating a coincident-face condition. Update the Y positions in timDety_even and timDety_odd in timing_detector.gmx so the outermost bar faces sit slightly inside s_containerHalfY, similar to the margin approach used in UpstreamTaggerFactory.cpp, rather than changing the container envelope. Ensure the adjusted grid still preserves symmetry around zero while avoiding exact contact with the 3250mm boundary.stage/subsystems/TimingDetector/src/TimingDetectorFactory.cpp-31-51 (1)
31-51: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd an explicit failure when the GMX file cannot be found.
resolveGmxPath()still returnstiming_detector.gmxif neither fallback exists, sobuild()hands a missing file toGmx2Geoand defers the error to a lower-level parser. Throw a descriptive exception before constructingGmx2Geo.🤖 Prompt for 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. In `@stage/subsystems/TimingDetector/src/TimingDetectorFactory.cpp` around lines 31 - 51, The GMX path resolution in resolveGmxPath still falls back to the default filename even when neither candidate exists, so TimingDetectorFactory::build ends up passing a missing file into Gmx2Geo. Update resolveGmxPath to explicitly detect the “not found” case and throw a descriptive exception before constructing Gmx2Geo, keeping the failure at TimingDetectorFactory/resolveGmxPath rather than deferring it to the parser.
🧹 Nitpick comments (19)
subsystems/MuonShield/README.md (1)
19-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a language to the fenced code block.
markdownlint flags this block (MD040) for missing a language specifier.
📝 Proposed fix
-``` +```text muon_shield (Air envelope, 4200 × 4600 × 27480 mm) centre z = 18.48 m ├─ block_0 (Iron, 3000 × 2000 × 2748 mm) ├─ block_1 (Iron, 3000 × 2000 × 2748 mm) ├─ … └─ block_9 (Iron, 3000 × 2000 × 2748 mm)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@subsystems/MuonShield/README.mdaround lines 19 - 25, Add a language
specifier to the fenced block in the MuonShield README so markdownlint MD040 is
satisfied; update the code fence around the muon_shield diagram to use a
descriptive language tag such as text, keeping the block contents unchanged.</details> <!-- cr-comment:v1:718f8e202038c1c176d9ea6a --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>subsystems/MuonShield/CMakeLists.txt (1)</summary><blockquote> `45-58`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Baked `MS_TOML_INSTALL_PATH` won't survive staged/relocated installs.** `${CMAKE_INSTALL_PREFIX}` is embedded as an absolute compile-time string. This breaks for `DESTDIR`-staged packaging (rpm/deb) or if the install tree is later relocated — the binary will still look for the config at the original build-time prefix. This mirrors the same pattern presumably used for `calo.toml`/`sbt.toml`, so it's a pre-existing project convention rather than something unique to this file. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@subsystems/MuonShield/CMakeLists.txtaround lines 45 - 58, The
MS_TOML_INSTALL_PATH definition in MuonShield is baking in CMAKE_INSTALL_PREFIX
as an absolute path, which will fail for staged or relocated installs. Update
the install-time lookup logic so it uses an install-root-relative path or
another relocatable mechanism consistent with the existing calo.toml/sbt.toml
pattern, and keep the fallback aligned with the MuonShield target’s config
loading behavior.</details> <!-- cr-comment:v1:cdef84e3ca18e656014ee3ef --> </blockquote></details> <details> <summary>subsystems/MuonShield/src/MuonShieldFactory.cpp (1)</summary><blockquote> `33-47`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _⚡ Quick win_ **Use `std::filesystem::path::is_absolute()` here.** `path[0] == '/'` is POSIX-only, so absolute Windows paths will miss the fast path and fall through to fallback lookup. The same resolver also appears in `CalorimeterFactory.cpp` and `DecayVolumeFactory.cpp`, so a shared helper would keep the behavior aligned. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@subsystems/MuonShield/src/MuonShieldFactory.cpparound lines 33 - 47, The
resolveTomlPath helper is using a POSIX-only slash check to detect absolute
paths, which breaks Windows path handling and should be replaced with
std::filesystem::path::is_absolute(). Update the absolute-path branch in
resolveTomlPath in MuonShieldFactory.cpp, and apply the same change to the
matching resolver logic in CalorimeterFactory.cpp and DecayVolumeFactory.cpp so
all factories behave consistently; if possible, factor the shared
path-resolution logic into one common helper.</details> <!-- cr-comment:v1:2733782706d5e83281bc6e8c --> </blockquote></details> <details> <summary>subsystems/MuonShield/include/MuonShield/MuonShieldFactory.h (1)</summary><blockquote> `40-45`: _🩺 Stability & Availability_ | _🔵 Trivial_ | _💤 Low value_ **Consider guarding against calling `centreZ_mm()` before `build()`.** The docs say the value is "valid only after build()", but nothing enforces it — a future caller invoking `centreZ_mm()` before `build()` would silently get `0.0` instead of an error. Not currently an issue since the one call site (`stage/src/SHiPGeometry.cpp`) calls `build()` first, but a `built_` flag with an assert/throw would make misuse fail loudly. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@subsystems/MuonShield/include/MuonShield/MuonShieldFactory.haround lines 40
- 45, Guard MuonShieldFactory::centreZ_mm() against being called before
MuonShieldFactory::build() has completed, since it currently returns
m_centreZ_mm silently even when the value is not yet valid. Add a built-state
check in MuonShieldFactory (for example a built_ flag set by build()) and make
centreZ_mm() fail loudly via an assert or exception when accessed too early,
while keeping the existing valid-after-build behavior for normal callers.</details> <!-- cr-comment:v1:e94fc92e440b8e481aeedd10 --> </blockquote></details> <details> <summary>stage/README.md (1)</summary><blockquote> `133-150`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Add a language hint to the fenced code block.** Static analysis flags this ASCII-tree block as missing a language identifier (MD040). <details> <summary>📝 Proposed fix</summary> ```diff -``` +```text geometry/ ├── include/SHiPGeometry/ # Public headers (SHiPGeometry, SHiPMaterials)🤖 Prompt for 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. In `@stage/README.md` around lines 133 - 150, The ASCII-tree block in the README is missing a fenced code language tag, triggering MD040. Update the fenced block in the geometry section to use a text-oriented language hint so the markdown parser recognizes it, and keep the existing tree content unchanged.Source: Linters/SAST tools
stage/CMakeLists.txt (1)
84-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate install rule for
build_geometry/validate_geometry.These two targets are already installed by
stage/apps/CMakeLists.txt(lines 17-20). Having the sameinstall(TARGETS ...)rule in two places is redundant and a maintenance hazard if the two diverge (e.g., differingEXPORTsets or destinations).♻️ Proposed fix — remove one of the duplicate rules
-install( - TARGETS build_geometry validate_geometry - RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR} -) -🤖 Prompt for 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. In `@stage/CMakeLists.txt` around lines 84 - 87, The install rule for build_geometry and validate_geometry is duplicated, so remove the redundant TARGETS install from stage/CMakeLists and keep the single source of truth in stage/apps/CMakeLists. Verify the remaining install(TARGETS ...) block still covers both targets with the intended RUNTIME DESTINATION and any other install/export settings used by those symbols.stage/src/SHiPMaterials.cpp (1)
85-268: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
.at()instead ofoperator[]for element lookups.All current element keys resolve correctly, but
m_elements["Name"]silently inserts a null entry on a typo instead of failing fast..at()would throw immediately, catching future maintenance errors at construction rather than passing a nullGeoElement*intoGeoMaterial::add().♻️ Example diff (Air material)
- air->add(m_elements["Nitrogen"], 0.755); - air->add(m_elements["Oxygen"], 0.231); - air->add(m_elements["Argon"], 0.014); + air->add(m_elements.at("Nitrogen"), 0.755); + air->add(m_elements.at("Oxygen"), 0.231); + air->add(m_elements.at("Argon"), 0.014);🤖 Prompt for 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. In `@stage/src/SHiPMaterials.cpp` around lines 85 - 268, The element lookups in SHiPMaterials::createMaterials() use m_elements["Name"], which can silently insert null entries if a key is misspelled. Replace these operator[] accesses with m_elements.at(...) throughout createMaterials() so missing elements fail fast before being passed into GeoMaterial::add(), using the existing material-building blocks like the Air, Concrete, and ArCO2_70_30 sections as reference points.stage/subsystems/DecayVolume/include/DecayVolume/SBTConfig.h (1)
35-54: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnenforced invariant:
container_thickness_mmshould equalwebHeight().Docs state
container_thickness_mmmust equal the H-beam clear web height (hbeam_height_mm - 2*hbeam_flange_thickness_mm), and the default (225 mm) satisfies it, but nothing inreadSBTConfigenforces this relationship. Editing either value independently would silently produce sensor containers that overlap or gap from the H-beam structure, with no build/test failure to catch it.Consider either deriving the container thickness from
webHeight()(dropping the duplicate field) or adding a tolerance check inreadSBTConfig, e.g.:if (std::abs(cfg.container_thickness_mm - cfg.webHeight()) > 1e-6) throw std::runtime_error( "SBTConfig: 'container_thickness_mm' must equal hbeam_height_mm - " "2*hbeam_flange_thickness_mm (the H-beam clear web height)");🤖 Prompt for 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. In `@stage/subsystems/DecayVolume/include/DecayVolume/SBTConfig.h` around lines 35 - 54, The SBTConfig invariant between container_thickness_mm and webHeight() is only documented, not enforced. Update readSBTConfig to validate that cfg.container_thickness_mm matches cfg.webHeight() within a small tolerance, or derive container_thickness_mm directly from webHeight() in SBTConfig to avoid duplicate sources of truth. Use the existing SBTConfig::webHeight() helper and the container_thickness_mm field to locate the check.stage/subsystems/Magnet/README.md (1)
11-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a language tag to the fenced geometry-tree block.
Flagged by markdownlint (MD040): the fenced block is missing a language identifier.
Proposed fix
-``` +```text SHiPMagnet (Air, 6500×8600×5000 mm)🤖 Prompt for 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. In `@stage/subsystems/Magnet/README.md` around lines 11 - 22, The fenced geometry-tree block in the Magnet README is missing a language identifier, triggering markdownlint MD040. Update the opening fence for the geometry-tree snippet to include an appropriate tag such as text, and keep the content otherwise unchanged. Use the fenced block near the SHiPMagnet diagram to make the fix easy to locate.Source: Linters/SAST tools
stage/subsystems/DecayVolume/src/SBTStructureBuilder.cpp (1)
112-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated rotation-frame construction between
placeHBeamInclinedand the inline lambda.The orthonormal-frame computation (
n/baxis derivation, degenerate-axis fallback,RotationMatrix3Dassembly) at Lines 138-176 is re-implemented nearly verbatim inside theplaceLongBeamlambda at Lines 313-332. Extracting a shared helper (e.g.,computeBeamFrame(x0,y0,z0,x1,y1,z1) -> {rotMat, halfLen}) would remove ~35 lines of duplicated vector math and reduce the risk of the two copies drifting apart.Also applies to: 306-343
🤖 Prompt for 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. In `@stage/subsystems/DecayVolume/src/SBTStructureBuilder.cpp` around lines 112 - 194, The orthonormal beam-frame construction is duplicated between placeHBeamInclined and the placeLongBeam lambda, including the degenerate-axis fallback and RotationMatrix3D setup. Extract that shared vector math into a helper such as computeBeamFrame (returning the rotation matrix and half-length), then have both placeHBeamInclined and placeLongBeam call it so the frame logic stays consistent and easier to maintain.stage/subsystems/Trackers/README.md (1)
21-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language hint to the fenced geometry-tree block.
Static analysis flags this fence for missing a language identifier (MD040).
Proposed fix
-``` +```text /SHiP/trackers (Air, 3000 × 3430 × 6000 mm half-extents)🤖 Prompt for 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. In `@stage/subsystems/Trackers/README.md` around lines 21 - 33, The geometry-tree fenced block in the Trackers README is missing a language identifier, triggering the MD040 markdown lint warning. Update the fence that wraps the /SHiP/trackers tree to include a text language hint, keeping the content unchanged; this is the only fenced block in this snippet and should be easy to locate by the /SHiP/trackers heading and nested station_<n>/view_<v>/envelope entries.Source: Linters/SAST tools
stage/subsystems/Cavern/test_cavern.cpp (1)
15-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest only covers the outer world box; cavern shape/cavities are unverified.
The subtraction chain producing
m_cavern(5 boolean operations) has no assertions — a regression there (e.g., wrong material, or a cavity subtraction silently failing) wouldn't be caught.♻️ Suggested additional coverage
CHECK(box->getXHalfLength() == 200000.0); CHECK(box->getYHalfLength() == 200000.0); CHECK(box->getZHalfLength() == 200000.0); + + // Verify the cavern (rock) volume was added and uses Concrete + REQUIRE(world->getNChildVols() > 0); + GeoPhysVol* cavern = dynamic_cast<GeoPhysVol*>(world->getChildVol(world->getNChildVols() - 1)); + REQUIRE(cavern != nullptr); + CHECK(cavern->getLogVol()->getMaterial()->getName() == "Concrete"); }🤖 Prompt for 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. In `@stage/subsystems/Cavern/test_cavern.cpp` around lines 15 - 25, The CavernFactory test only verifies the outer world box and misses the boolean subtraction chain that builds m_cavern, so add assertions that exercise the actual cavern/cavity geometry. Extend test_cavern.cpp in TEST_CASE("CavernWorldVolume", ...) to inspect the returned world volume and validate the nested GeoShape/boolean structure produced by SHiPGeometry::CavernFactory::build(), including the expected cavity subtractions and resulting material/shape configuration so a broken subtraction step is caught.stage/subsystems/MuonShield/include/MuonShield/MuonShieldConfig.h (1)
20-50: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider guarding
blockThicknessZ_mm()againstn_sections == 0.
envelopeLengthZ_mm() / n_sectionsdivides byn_sectionsunconditionally. This is currently safe only becausereadMuonShieldConfigvalidatesn_sections > 0before any caller can use this struct meaningfully — but aMuonShieldConfigcan also be default- or hand-constructed without going through the parser, in which case nothing preventsn_sectionsfrom being set to0before calling this helper.🤖 Prompt for 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. In `@stage/subsystems/MuonShield/include/MuonShield/MuonShieldConfig.h` around lines 20 - 50, Guard MuonShieldConfig::blockThicknessZ_mm() against n_sections being zero, since it currently divides unconditionally by n_sections and can be called on a default- or manually-constructed MuonShieldConfig outside readMuonShieldConfig validation. Update the helper to handle the invalid zero case safely, and keep the fix localized to the MuonShieldConfig struct so callers of envelopeLengthZ_mm() and blockThicknessZ_mm() do not need to change.stage/subsystems/MuonShield/include/MuonShield/MuonShieldFactory.h (1)
38-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStateful ordering between
build()andcentreZ_mm()isn't enforced.
centreZ_mm()silently returns the default0.0(line 55) if called beforebuild(), rather than failing loudly. Since correct world placement depends on this value, consider bundling the physical volume and the centre offset into a single return type frombuild()(or asserting/throwing incentreZ_mm()if not yet populated) to remove the temporal coupling.🤖 Prompt for 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. In `@stage/subsystems/MuonShield/include/MuonShield/MuonShieldFactory.h` around lines 38 - 49, The MuonShieldFactory API currently allows centreZ_mm() to return a default 0.0 before build(), which hides incorrect call ordering. Update MuonShieldFactory so the build()/centreZ_mm() temporal coupling is enforced by either returning the center offset together with the GeoPhysVol from build() via a single result type, or by making centreZ_mm() assert/throw until the value has been populated. Use the existing MuonShieldFactory, build(), and centreZ_mm() symbols to keep the contract explicit and prevent silent fallback values.stage/subsystems/MuonShield/src/MuonShieldFactory.cpp (1)
54-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate path-resolution call.
build()(Line 61) callsresolveTomlPath(m_configPath)directly instead of reusingresolvedConfigPath()(Line 54-56), duplicating the resolution logic path.♻️ Proposed fix
GeoPhysVol* MuonShieldFactory::build() { - const MuonShieldConfig cfg = readMuonShieldConfig(resolveTomlPath(m_configPath)); + const MuonShieldConfig cfg = readMuonShieldConfig(resolvedConfigPath());🤖 Prompt for 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. In `@stage/subsystems/MuonShield/src/MuonShieldFactory.cpp` around lines 54 - 61, `MuonShieldFactory::build()` is re-resolving `m_configPath` instead of reusing the existing `resolvedConfigPath()` helper, which duplicates the same path logic. Update `build()` to call `resolvedConfigPath()` when passing the config path into `readMuonShieldConfig`, so the path resolution is centralized in `MuonShieldFactory::resolvedConfigPath()` and not repeated.stage/subsystems/Target/test_target.cpp (1)
15-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest only covers the outer vacuum box; sub-component placements are untested.
This test verifies the top-level
vacuum_boxdimensions but doesn't exercise any of the manually re-derived offsets for the proximity/top/bottom shielding, vessel, caps, or the 19 target slabs inTargetFactory.cpp(Lines 34-108) — the parts most likely to contain an error during a GDML→code translation. Consider adding assertions on a few child transform positions (e.g. vessel, front/back caps) to catch regressions in the offset math.🤖 Prompt for 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. In `@stage/subsystems/Target/test_target.cpp` around lines 15 - 26, The TargetBuilds test only checks the outer vacuum_box dimensions and leaves the manually computed placement offsets in TargetFactory unverified. Update the test around SHiPMaterials and SHiPMGeometry::TargetFactory::build() to also inspect selected child placements from the returned GeoPhysVol, especially the vessel and front/back cap transforms, so regressions in the offset math for the shielding, vessel, caps, and target slabs are caught even if the top-level box still passes.stage/subsystems/Calorimeter/README.md (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMislabeled dimensions: these are full extents, not half-extents.
CalorimeterFactory.hdefiness_containerHalfX/Y/Z= 3000/3500/1450 mm — the values documented here (6000×7000×2900) are the corresponding full box sizes, not half-extents.✏️ Suggested fix
-/SHiP/calorimeter (Air, 6000 × 7000 × 2900 mm half-extents fixed by SHiPGeometry) +/SHiP/calorimeter (Air, 6000 × 7000 × 2900 mm full extents / 3000 × 3500 × 1450 mm half-extents, fixed by SHiPGeometry)🤖 Prompt for 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. In `@stage/subsystems/Calorimeter/README.md` at line 18, The Calorimeter README currently labels the container dimensions as half-extents, but the documented 6000 × 7000 × 2900 mm values are actually the full sizes derived from CalorimeterFactory.h constants like s_containerHalfX, s_containerHalfY, and s_containerHalfZ. Update the description in the calorimeter section to say full extents or otherwise clarify that these are the full box dimensions, so it matches the definitions used by CalorimeterFactory.stage/subsystems/TimingDetector/README.md (1)
11-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language identifier to fenced code block.
markdownlint flags the geometry-tree block for missing a language hint.
📝 Proposed fix
-``` +```text Timing_Detector (Air, 5500×6500×500 mm) └─ 330 × scintillator bars (1400×60×10 mm each) built from GeoModelXML prototype</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@stage/subsystems/TimingDetector/README.mdaround lines 11 - 15, The fenced
geometry-tree block in the TimingDetector README is missing a language
identifier, which triggers markdownlint. Update the fenced block to use an
appropriate language hint such as text while keeping the existing content and
formatting intact.</details> <!-- cr-comment:v1:2031c3e9f8f905a3d9feeb4e --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>stage/subsystems/TimingDetector/CMakeLists.txt (1)</summary><blockquote> `31-41`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Fragile assumption on `GeoModelTools_DIR` layout.** `_gt_prefix` is derived by walking three levels up from `GeoModelTools_DIR`, assuming the standard `<prefix>/lib/cmake/GeoModelTools` layout. This holds for typical CMake config-package installs, but breaks silently (degrading to just the warning at Lines 37-40) for `lib64`, multi-level, or non-standard install layouts, since there is no fallback search via `find_file`. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@stage/subsystems/TimingDetector/CMakeLists.txtaround lines 31 - 41, The
geomodel.dtd lookup in the TimingDetector CMake logic is too dependent on
walking up from GeoModelTools_DIR, so replace the hardcoded prefix derivation in
the geomodel.dtd copy block with a proper search using find_file or an
equivalent fallback search path. Keep the existing configure_file copy step in
place, but make it consume the discovered DTD path from the search result
instead of assuming the /share/GeoModelXml layout, and preserve the
warning path only when the search truly fails.</details> <!-- cr-comment:v1:1086fc4fd1da579c13889b46 --> </blockquote></details> </blockquote></details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `95d2a274-90e0-42d0-8d32-93be225a4c61` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 203b9d2f0ed89ccec49c68f37601c39285982de7 and 1fff4ce63422edf9e7be42f8802abe777effef22. </details> <details> <summary>⛔ Files ignored due to path filters (4)</summary> * `geometry.tar.gz` is excluded by `!**/*.gz` * `geometry_muonshield.tar.gz` is excluded by `!**/*.gz` * `stage/pixi.lock` is excluded by `!**/*.lock` * `stage/subsystem_envelopes.csv` is excluded by `!**/*.csv` </details> <details> <summary>📒 Files selected for processing (101)</summary> * `src/SHiPGeometry.cpp` * `stage/CMakeLists.txt` * `stage/LICENSES/LGPL-3.0-or-later.txt` * `stage/README.md` * `stage/REUSE.toml` * `stage/apps/CMakeLists.txt` * `stage/cliff.toml` * `stage/cmake/SHiPGeometryConfig.cmake.in` * `stage/committed.toml` * `stage/doxygen/Doxyfile` * `stage/include/SHiPGeometry/SHiPGeometry.h` * `stage/include/SHiPGeometry/SHiPMaterials.h` * `stage/pixi.toml` * `stage/renovate.json` * `stage/scripts/release.sh` * `stage/src/CMakeLists.txt` * `stage/src/SHiPGeometry.cpp` * `stage/src/SHiPMaterials.cpp` * `stage/subsystems/CMakeLists.txt` * `stage/subsystems/Calorimeter/CMakeLists.txt` * `stage/subsystems/Calorimeter/README.md` * `stage/subsystems/Calorimeter/calo.toml` * `stage/subsystems/Calorimeter/include/Calorimeter/CaloBarLayer.h` * `stage/subsystems/Calorimeter/include/Calorimeter/CaloFibreHPLayer.h` * `stage/subsystems/Calorimeter/include/Calorimeter/CalorimeterConfig.h` * `stage/subsystems/Calorimeter/include/Calorimeter/CalorimeterFactory.h` * `stage/subsystems/Calorimeter/src/CaloBarLayer.cpp` * `stage/subsystems/Calorimeter/src/CaloFibreHPLayer.cpp` * `stage/subsystems/Calorimeter/src/CalorimeterConfig.cpp` * `stage/subsystems/Calorimeter/src/CalorimeterFactory.cpp` * `stage/subsystems/Calorimeter/test_calorimeter.cpp` * `stage/subsystems/Cavern/CMakeLists.txt` * `stage/subsystems/Cavern/README.md` * `stage/subsystems/Cavern/include/Cavern/CavernFactory.h` * `stage/subsystems/Cavern/src/CavernFactory.cpp` * `stage/subsystems/Cavern/test_cavern.cpp` * `stage/subsystems/DecayVolume/CMakeLists.txt` * `stage/subsystems/DecayVolume/README.md` * `stage/subsystems/DecayVolume/include/DecayVolume/DecayVolumeFactory.h` * `stage/subsystems/DecayVolume/include/DecayVolume/SBTConfig.h` * `stage/subsystems/DecayVolume/include/DecayVolume/SBTSensorBuilder.h` * `stage/subsystems/DecayVolume/include/DecayVolume/SBTStructureBuilder.h` * `stage/subsystems/DecayVolume/sbt.toml` * `stage/subsystems/DecayVolume/src/DecayVolumeFactory.cpp` * `stage/subsystems/DecayVolume/src/SBTConfig.cpp` * `stage/subsystems/DecayVolume/src/SBTSensorBuilder.cpp` * `stage/subsystems/DecayVolume/src/SBTStructureBuilder.cpp` * `stage/subsystems/DecayVolume/test_decayvolume.cpp` * `stage/subsystems/Magnet/CMakeLists.txt` * `stage/subsystems/Magnet/README.md` * `stage/subsystems/Magnet/include/Magnet/MagnetFactory.h` * `stage/subsystems/Magnet/src/MagnetFactory.cpp` * `stage/subsystems/Magnet/test_magnet.cpp` * `stage/subsystems/MuonShield/CMakeLists.txt` * `stage/subsystems/MuonShield/MS.toml` * `stage/subsystems/MuonShield/README.md` * `stage/subsystems/MuonShield/include/MuonShield/MuonShieldConfig.h` * `stage/subsystems/MuonShield/include/MuonShield/MuonShieldFactory.h` * `stage/subsystems/MuonShield/src/MuonShieldConfig.cpp` * `stage/subsystems/MuonShield/src/MuonShieldFactory.cpp` * `stage/subsystems/MuonShield/test_muonshield.cpp` * `stage/subsystems/NeutrinoDetector/CMakeLists.txt` * `stage/subsystems/NeutrinoDetector/README.md` * `stage/subsystems/NeutrinoDetector/include/NeutrinoDetector/NeutrinoDetectorFactory.h` * `stage/subsystems/NeutrinoDetector/src/NeutrinoDetectorFactory.cpp` * `stage/subsystems/NeutrinoDetector/test_neutrinodetector.cpp` * `stage/subsystems/Target/CMakeLists.txt` * `stage/subsystems/Target/README.md` * `stage/subsystems/Target/include/Target/TargetFactory.h` * `stage/subsystems/Target/src/TargetFactory.cpp` * `stage/subsystems/Target/test_target.cpp` * `stage/subsystems/TimingDetector/CMakeLists.txt` * `stage/subsystems/TimingDetector/README.md` * `stage/subsystems/TimingDetector/data/timing_detector.gmx` * `stage/subsystems/TimingDetector/include/TimingDetector/SHiPTimingDetInterface.h` * `stage/subsystems/TimingDetector/include/TimingDetector/TimingDetectorFactory.h` * `stage/subsystems/TimingDetector/src/TimingDetectorFactory.cpp` * `stage/subsystems/TimingDetector/test_timingdetector.cpp` * `stage/subsystems/Trackers/CMakeLists.txt` * `stage/subsystems/Trackers/README.md` * `stage/subsystems/Trackers/include/Trackers/TrackersFactory.h` * `stage/subsystems/Trackers/src/TrackersFactory.cpp` * `stage/subsystems/Trackers/test_trackers.cpp` * `stage/subsystems/UpstreamTagger/CMakeLists.txt` * `stage/subsystems/UpstreamTagger/README.md` * `stage/subsystems/UpstreamTagger/include/UpstreamTagger/SHiPUBTManager.h` * `stage/subsystems/UpstreamTagger/include/UpstreamTagger/UpstreamTaggerFactory.h` * `stage/subsystems/UpstreamTagger/src/UpstreamTaggerFactory.cpp` * `stage/subsystems/UpstreamTagger/test_upstreamtagger.cpp` * `stage/tests/CMakeLists.txt` * `stage/tests/test_builder.cpp` * `stage/tests/test_consistency.cpp` * `stage/tests/test_materials.cpp` * `subsystems/MuonShield/CMakeLists.txt` * `subsystems/MuonShield/MS.toml` * `subsystems/MuonShield/README.md` * `subsystems/MuonShield/include/MuonShield/MuonShieldConfig.h` * `subsystems/MuonShield/include/MuonShield/MuonShieldFactory.h` * `subsystems/MuonShield/src/MuonShieldConfig.cpp` * `subsystems/MuonShield/src/MuonShieldFactory.cpp` * `subsystems/MuonShield/test_muonshield.cpp` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| cmake_minimum_required(VERSION 3.16) | ||
|
|
||
| project( | ||
| SHiPGeometry | ||
| VERSION 0.2.1 | ||
| DESCRIPTION "SHiP detector geometry implementation using GeoModel" | ||
| LANGUAGES CXX | ||
| ) | ||
|
|
||
| # C++23: required by Phlex | ||
| set(CMAKE_CXX_STANDARD 23) | ||
| set(CMAKE_CXX_STANDARD_REQUIRED ON) | ||
| set(CMAKE_CXX_EXTENSIONS OFF) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does CMake 3.16 support CMAKE_CXX_STANDARD 23?
💡 Result:
No, CMake 3.16 does not support CMAKE_CXX_STANDARD 23 [1]. Support for C++23 in the CMAKE_CXX_STANDARD variable and the CXX_STANDARD target property was introduced in CMake 3.20 [2][3]. In CMake 3.16, the supported values for CXX_STANDARD are limited to 98, 11, 14, 17, and 20 [1].
Citations:
- 1: https://cmake.org/cmake/help/v3.16/prop_tgt/CXX_STANDARD.html
- 2: https://cmake.org/cmake/help/v3.23/prop_tgt/CXX_STANDARD.html
- 3: https://cmake.org/cmake/help/v4.3/prop_tgt/CXX_STANDARD.html
Raise the CMake minimum to 3.20 CMAKE_CXX_STANDARD 23 is only supported starting in CMake 3.20, so 3.16–3.19 can leave the project configured without C++23.
🤖 Prompt for 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.
In `@stage/CMakeLists.txt` around lines 4 - 16, The stage/CMakeLists.txt
configuration currently sets the minimum CMake version too low for the
CMAKE_CXX_STANDARD 23 setting used in the top-level project block. Update the
cmake_minimum_required declaration in stage/CMakeLists.txt to require CMake 3.20
or newer so the CXX standard setting is reliably honored during configuration.
| // ── Air container ──────────────────────────────────────────────────── | ||
| // The container is the experiment's fixed envelope allocation for the | ||
| // decay region (s_halfX/Y/Z). The SBT geometry comes from sbt.toml, so a | ||
| // future edit there could in principle outgrow it. Guard against that: | ||
| // bound the outermost structure reach from the config and fail loudly if | ||
| // it would pierce the envelope, rather than silently overlapping. | ||
| // | ||
| // Reference description: rectangular frustum, 2.0 x 3.0 m entrance -> | ||
| // 4.0 x 6.0 m exit, 50 m long. With the default config the reaches are | ||
| // ~2130 / ~3259 / ~25130 mm, inside the 2200 / 3300 / 25200 envelope. | ||
| { | ||
| const double growthY = (cfg.y_half_exit_mm - cfg.y_half_entrance_mm) / cfg.total_length_mm; | ||
| // X: vertical columns sit at x_half_exit with a half-flange overhang. | ||
| const double reachX = cfg.x_half_exit_mm + 0.5 * cfg.hbeam_flange_width_mm; | ||
| // Y: top/bottom cross-beams are shifted a full beam-height above | ||
| // y_half_exit (plus a small frustum-growth term); +10 pads the | ||
| // assembly standoff and is a safe upper bound on the real ~3259 mm. | ||
| const double reachY = cfg.y_half_exit_mm + cfg.hbeam_height_mm + | ||
| 0.5 * cfg.hbeam_flange_width_mm * growthY + 10.0; | ||
| // Z: the 50 m structure spans [z_entrance, z_entrance + total_length]; | ||
| // cross-beam flanges extend half a flange-width beyond the end rows. | ||
| const double reachZ = std::max(std::abs(cfg.z_entrance_mm), | ||
| std::abs(cfg.z_entrance_mm + cfg.total_length_mm)) + | ||
| 0.5 * cfg.hbeam_flange_width_mm; | ||
|
|
||
| if (reachX > s_halfX || reachY > s_halfY || reachZ > s_halfZ) { | ||
| throw std::runtime_error( | ||
| "DecayVolumeFactory: configured SBT (half-reach " + std::to_string(reachX) + " x " + | ||
| std::to_string(reachY) + " x " + std::to_string(reachZ) + | ||
| " mm) does not fit the decay-volume envelope (" + std::to_string(s_halfX) + " x " + | ||
| std::to_string(s_halfY) + " x " + std::to_string(s_halfZ) + | ||
| " mm); adjust sbt.toml or the container size."); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Envelope guard doesn't validate entrance/exit half-widths against the helium inset.
The guard at Line 103 checks that the structure's outer reach fits the fixed envelope, but nothing validates that cfg.x_half_entrance_mm/y_half_entrance_mm/x_half_exit_mm/y_half_exit_mm exceed inset = container_thickness_mm + helium_clearance_mm (Line 120). SBTConfig::readSBTConfig only checks each of these fields individually for > 0, not their relationship to the inset. If a future sbt.toml edit sets a small entrance/exit half-width, dx1/dy1/dx2/dy2 (Lines 122-125) go negative, producing a degenerate/invalid GeoTrap.
🛡️ Proposed fix: add an inset-consistency check
const double inset = cfg.container_thickness_mm + cfg.helium_clearance_mm;
+ if (cfg.x_half_entrance_mm <= inset || cfg.y_half_entrance_mm <= inset ||
+ cfg.x_half_exit_mm <= inset || cfg.y_half_exit_mm <= inset) {
+ throw std::runtime_error(
+ "DecayVolumeFactory: entrance/exit half-widths must exceed container_thickness_mm + "
+ "helium_clearance_mm");
+ }
const double dz = 0.5 * cfg.total_length_mm * mm;🤖 Prompt for 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.
In `@stage/subsystems/DecayVolume/src/DecayVolumeFactory.cpp` around lines 78 -
111, The envelope check in DecayVolumeFactory only verifies the outer reach
against s_halfX/s_halfY/s_halfZ, but it still allows invalid trap dimensions if
the entrance/exit half-widths are smaller than the helium inset. Add a
consistency guard near the existing reach check in DecayVolumeFactory::build
(before computing dx1/dy1/dx2/dy2) that compares cfg.x_half_entrance_mm,
cfg.y_half_entrance_mm, cfg.x_half_exit_mm, and cfg.y_half_exit_mm against inset
= container_thickness_mm + helium_clearance_mm, and throw if any are too small.
Keep the error message specific so future sbt.toml edits that would create a
degenerate GeoTrap are caught early.
| target_include_directories( | ||
| TimingDetector | ||
| PUBLIC | ||
| $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include> | ||
| $<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/include> | ||
| $<INSTALL_INTERFACE:include> | ||
| ) | ||
|
|
||
| target_link_libraries( | ||
| TimingDetector | ||
| PUBLIC GeoModelCore::GeoModelKernel | ||
| PRIVATE GeoModelTools::GeoModelXml | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Missing SHiPGeometry link on the TimingDetector target.
target_include_directories (Line 10) pulls in ${CMAKE_SOURCE_DIR}/include specifically so TimingDetectorFactory.cpp can #include "SHiPGeometry/SHiPMaterials.h", but target_link_libraries (Lines 14-18) never links the SHiPGeometry library that implements SHiPMaterials. The test target below (Line 73) has to add SHiPGeometry explicitly to link successfully, confirming the main library is missing this dependency. Any other consumer that links only TimingDetector (without independently adding SHiPGeometry) risks unresolved-symbol link errors.
🔧 Proposed fix
target_link_libraries(
TimingDetector
- PUBLIC GeoModelCore::GeoModelKernel
+ PUBLIC GeoModelCore::GeoModelKernel SHiPGeometry
PRIVATE GeoModelTools::GeoModelXml
)📝 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.
| target_include_directories( | |
| TimingDetector | |
| PUBLIC | |
| $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include> | |
| $<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/include> | |
| $<INSTALL_INTERFACE:include> | |
| ) | |
| target_link_libraries( | |
| TimingDetector | |
| PUBLIC GeoModelCore::GeoModelKernel | |
| PRIVATE GeoModelTools::GeoModelXml | |
| ) | |
| target_include_directories( | |
| TimingDetector | |
| PUBLIC | |
| $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include> | |
| $<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/include> | |
| $<INSTALL_INTERFACE:include> | |
| ) | |
| target_link_libraries( | |
| TimingDetector | |
| PUBLIC GeoModelCore::GeoModelKernel SHiPGeometry | |
| PRIVATE GeoModelTools::GeoModelXml | |
| ) |
🤖 Prompt for 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.
In `@stage/subsystems/TimingDetector/CMakeLists.txt` around lines 6 - 18, The
TimingDetector target is missing its direct dependency on SHiPGeometry, which is
needed for TimingDetectorFactory.cpp to use SHiPMaterials. Update the
target_link_libraries block for TimingDetector to link SHiPGeometry alongside
the existing GeoModelCore::GeoModelKernel and GeoModelTools::GeoModelXml
entries, so consumers of TimingDetector do not need to add that library
separately.
| # Integration test: build geometry into SQLite then run overlap check | ||
| add_test( | ||
| NAME OverlapCheck | ||
| COMMAND | ||
| bash -c | ||
| "$<TARGET_FILE:build_geometry> ship_test_overlap.db && | ||
| $<TARGET_FILE:validate_geometry> ship_test_overlap.db" | ||
| WORKING_DIRECTORY ${CMAKE_BINARY_DIR} | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -a CMakeLists.txt stage/apps | xargs grep -n "build_geometry\|validate_geometry"Repository: ShipSoft/Geometry
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Top-level directories:\n'
find . -maxdepth 2 -type d | sort | sed -n '1,120p'
printf '\nCMakeLists files mentioning build_geometry/validate_geometry:\n'
rg -n --hidden --glob 'CMakeLists.txt' 'build_geometry|validate_geometry' .
printf '\nFiles named CMakeLists.txt under stage:\n'
fd -a CMakeLists.txt stage .Repository: ShipSoft/Geometry
Length of output: 2129
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'apps/CMakeLists.txt:\n'
cat -n apps/CMakeLists.txt
printf '\nvalidate_geometry.cpp:\n'
cat -n apps/validate_geometry.cpp
printf '\ntests/CMakeLists.txt around OverlapCheck:\n'
sed -n '20,40p' tests/CMakeLists.txt
printf '\nCI workflows mentioning tests:\n'
rg -n "ctest|bash|ubuntu|windows|macos" .github/workflows -SRepository: ShipSoft/Geometry
Length of output: 11163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'build_geometry.cpp:\n'
cat -n apps/build_geometry.cpp
printf '\nRoot CMakeLists relevant lines:\n'
sed -n '70,95p' CMakeLists.txt
printf '\nWorkflow files:\n'
fd -a .github/workflows -e yml -e yaml .
printf '\nWorkflow excerpts mentioning runs-on / bash / ctest:\n'
rg -n "runs-on|bash|ctest|ubuntu|windows|macos" .github/workflows -SRepository: ShipSoft/Geometry
Length of output: 2714
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '.github/workflows files:\n'
fd . .github/workflows -t f
printf '\nWorkflow excerpts mentioning shells / runners / ctest:\n'
rg -n "runs-on|shell:|bash|ctest|ubuntu|windows|macos" .github/workflows -S
printf '\nWhere validate_geometry decides its exit code:\n'
rg -n "return 0|return 1|overlap|validateGeometry|isValid" apps/validate_geometry.cpp -n -SRepository: ShipSoft/Geometry
Length of output: 396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'build-test.yml:\n'
cat -n .github/workflows/build-test.yml
printf '\nrelease.yml:\n'
cat -n .github/workflows/release.yml | sed -n '1,220p'Repository: ShipSoft/Geometry
Length of output: 1162
Make validate_geometry fail on invalid geometry
build_geometry and validate_geometry both exist, but validate_geometry.cpp only prints a report and main() always returns 0. As written, OverlapCheck can pass even when the geometry is invalid or overlaps are present.
🤖 Prompt for 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.
In `@stage/tests/CMakeLists.txt` around lines 24 - 32, `validate_geometry`
currently reports issues but never fails the test because `main()` always
returns 0. Update `validate_geometry.cpp` so `main()` returns a non-zero exit
code whenever invalid geometry or overlaps are detected, and keep 0 only for the
all-clear case. Use the existing validation/reporting logic in
`validate_geometry` to drive the exit status so the `OverlapCheck` test
correctly fails on bad geometry.
| double getHalfZ(const GeoVPhysVol* vol) { | ||
| const auto* box = dynamic_cast<const GeoBox*>(vol->getLogVol()->getShape()); | ||
| if (box) { | ||
| return box->getZHalfLength(); | ||
| } | ||
| // For non-box shapes (e.g. GeoFullPhysVol with GeoBox), try the tube | ||
| const auto* tube = dynamic_cast<const GeoTube*>(vol->getLogVol()->getShape()); | ||
| if (tube) { | ||
| return tube->getZHalfLength(); | ||
| } | ||
| return 0.0; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Silent 0.0 fallback in getHalfZ can mask real overlaps.
Returning 0.0 for any shape that isn't GeoBox/GeoTube silently disables the overlap/order checks for that subsystem rather than failing loudly. If any top-level subsystem shape (e.g. a boolean/union shape) isn't one of these two, NoUnexpectedZOverlaps and Z-order checks will pass vacuously instead of catching real problems.
Consider failing the test (e.g. FAIL/REQUIRE) or logging a clear diagnostic when the shape type is unrecognized, so silent false negatives don't creep in as subsystems evolve.
Proposed fix
double getHalfZ(const GeoVPhysVol* vol) {
const auto* box = dynamic_cast<const GeoBox*>(vol->getLogVol()->getShape());
if (box) {
return box->getZHalfLength();
}
// For non-box shapes (e.g. GeoFullPhysVol with GeoBox), try the tube
const auto* tube = dynamic_cast<const GeoTube*>(vol->getLogVol()->getShape());
if (tube) {
return tube->getZHalfLength();
}
- return 0.0;
+ FAIL("Unsupported shape type for subsystem " << vol->getLogVol()->getName()
+ << "; extend getHalfZ to handle it.");
+ return 0.0;
}📝 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.
| double getHalfZ(const GeoVPhysVol* vol) { | |
| const auto* box = dynamic_cast<const GeoBox*>(vol->getLogVol()->getShape()); | |
| if (box) { | |
| return box->getZHalfLength(); | |
| } | |
| // For non-box shapes (e.g. GeoFullPhysVol with GeoBox), try the tube | |
| const auto* tube = dynamic_cast<const GeoTube*>(vol->getLogVol()->getShape()); | |
| if (tube) { | |
| return tube->getZHalfLength(); | |
| } | |
| return 0.0; | |
| } | |
| double getHalfZ(const GeoVPhysVol* vol) { | |
| const auto* box = dynamic_cast<const GeoBox*>(vol->getLogVol()->getShape()); | |
| if (box) { | |
| return box->getZHalfLength(); | |
| } | |
| // For non-box shapes (e.g. GeoFullPhysVol with GeoBox), try the tube | |
| const auto* tube = dynamic_cast<const GeoTube*>(vol->getLogVol()->getShape()); | |
| if (tube) { | |
| return tube->getZHalfLength(); | |
| } | |
| FAIL("Unsupported shape type for subsystem " << vol->getLogVol()->getName() | |
| << "; extend getHalfZ to handle it."); | |
| return 0.0; | |
| } |
🤖 Prompt for 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.
In `@stage/tests/test_consistency.cpp` around lines 34 - 45, The fallback in
getHalfZ currently returns 0.0 for unrecognized shapes, which can make the
Z-overlap and ordering checks in NoUnexpectedZOverlaps pass silently. Update
getHalfZ to explicitly handle only the supported shape types and fail loudly or
emit a clear diagnostic when vol->getLogVol()->getShape() is neither GeoBox nor
GeoTube, so unexpected subsystem shapes do not bypass the test logic.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Could you please reply to coderabbit where it did not identify the comment as resolved and also check the minor and nitpick comments that weren't posted as inline comments? |
|
I'll review this myself ASAP |
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
|
Please remove the two tarballs from this PR. |
040dc39 to
9b4b44a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
subsystems/MuonShield/README.md (1)
29-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a language identifier to the fenced code block.
Flagged by markdownlint (MD040).
📝 Proposed fix
-``` +```text muon_shield (Air envelope) ├─ block_0 (Iron, upstream of the gap) └─ block_1 (Iron, downstream of the gap)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@subsystems/MuonShield/README.mdaround lines 29 - 33, Add a language
identifier to the fenced Markdown block in the MuonShield README to satisfy
MD040; update the existing fenced snippet around the muon_shield diagram so it
uses a text-style fence rather than an unlabeled code block.</details> <!-- cr-comment:v1:41270a496e0eab1dfcd98e2a --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>subsystems/MuonShield/src/MuonShieldFactory.cpp (1)</summary><blockquote> `38-50`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Prefer `std::filesystem::path::is_absolute()` over manual `'/'` check.** More idiomatic and future-proof than a manual first-character check, since `<filesystem>` is already included. <details> <summary>♻️ Proposed refactor</summary> ```diff static std::string resolveTomlPath(const std::string& path) { - if (!path.empty() && path[0] == '/') + if (!path.empty() && std::filesystem::path(path).is_absolute()) return path; // already absolute🤖 Prompt for 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. In `@subsystems/MuonShield/src/MuonShieldFactory.cpp` around lines 38 - 50, The path resolution helper in resolveTomlPath is using a manual first-character check to detect absolute paths, which is less idiomatic than the filesystem API already available. Update the absolute-path branch in resolveTomlPath to use std::filesystem::path::is_absolute() on the input path, while keeping the existing fallback lookup behavior for the relative, source-tree, and install paths unchanged.
🤖 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.
Nitpick comments:
In `@subsystems/MuonShield/README.md`:
- Around line 29-33: Add a language identifier to the fenced Markdown block in
the MuonShield README to satisfy MD040; update the existing fenced snippet
around the muon_shield diagram so it uses a text-style fence rather than an
unlabeled code block.
In `@subsystems/MuonShield/src/MuonShieldFactory.cpp`:
- Around line 38-50: The path resolution helper in resolveTomlPath is using a
manual first-character check to detect absolute paths, which is less idiomatic
than the filesystem API already available. Update the absolute-path branch in
resolveTomlPath to use std::filesystem::path::is_absolute() on the input path,
while keeping the existing fallback lookup behavior for the relative,
source-tree, and install paths unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58ad5b46-a262-47cf-af63-cc7c1d2a2d15
📒 Files selected for processing (10)
src/SHiPGeometry.cppsubsystems/MuonShield/MS.tomlsubsystems/MuonShield/README.mdsubsystems/MuonShield/include/MuonShield/MuonShieldConfig.hsubsystems/MuonShield/include/MuonShield/MuonShieldFactory.hsubsystems/MuonShield/src/MuonShieldConfig.cppsubsystems/MuonShield/src/MuonShieldFactory.cppsubsystems/MuonShield/test_muonshield.cppsubsystems/NeutrinoDetector/README.mdtests/test_consistency.cpp
✅ Files skipped from review due to trivial changes (1)
- subsystems/NeutrinoDetector/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- subsystems/MuonShield/include/MuonShield/MuonShieldFactory.h
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
subsystems/MuonShield/test_muonshield.cpp (1)
86-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test exercises the
rotationblock field.Rotation is a documented feature (README Lines 68–75) and parsed by
readFixedArray<3>inMuonShieldConfig.cpp, but no test verifies that a rotated block produces the expectedGeoBox/GeoTrdorientation. Consider adding a test with a non-zero rotation to validate the extrinsic X→Y→Z transform is applied correctly aboutstart.🤖 Prompt for 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. In `@subsystems/MuonShield/test_muonshield.cpp` around lines 86 - 103, The MuonShield test coverage is missing validation for the block-level rotation field, so add a new case in test_muonshield.cpp alongside MuonShieldTaperMakesTrd that configures a non-zero rotation and asserts the resulting GeoBox or GeoTrd is oriented correctly. Use MuonShieldFactory::build, the parsed rotation from MuonShieldConfig.cpp, and the block’s start point as the pivot, then verify the extrinsic X→Y→Z transform is reflected in the child volume’s shape/orientation.subsystems/MuonShield/src/MuonShieldConfig.cpp (2)
54-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
readNumericandnodeToNumberduplicate the same int/float-to-double logic.Both functions accept either a TOML float or integer and return
double, differing only in input type (toml::node_view<toml::node>vsconst toml::node*). A single templated helper or havingreadNumericdelegate tonodeToNumberwould eliminate the duplication.🤖 Prompt for 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. In `@subsystems/MuonShield/src/MuonShieldConfig.cpp` around lines 54 - 72, The int/float-to-double conversion logic is duplicated between readNumeric and nodeToNumber in MuonShieldConfig.cpp. Refactor so there is a single helper that performs the TOML numeric extraction and conversion, then have the other function delegate to it; keep the existing error text behavior intact and preserve the current signatures of readNumeric and nodeToNumber if they’re used elsewhere.
162-163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
kDegToRadduplicates thedegToRadconstant inMuonShieldFactory.cppline 78.Both files define the same
3.14159265358979323846 / 180.0constant with different names. A shared constant (e.g., inMuonShieldConfig.hor a common header) would prevent drift and improve consistency.🤖 Prompt for 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. In `@subsystems/MuonShield/src/MuonShieldConfig.cpp` around lines 162 - 163, The degree-to-radian constant is duplicated between MuonShieldConfig.cpp and MuonShieldFactory.cpp under different names, so centralize it in a shared place such as MuonShieldConfig.h or a common MuonShield header and update both MuonShieldConfig and MuonShieldFactory to use the same symbol. Keep the existing kEps local if needed, but remove the duplicate degToRad/kDegToRad definition so there is a single source of truth for the conversion constant.
🤖 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/MuonShield/README.md`:
- Around line 45-52: The MuonShield config defaults in the README do not match
the actual auto-sized geometry produced by the envelope factory, so update the
table to reflect that these are parser fallbacks rather than the built
dimensions. In the MuonShield README section with the config table, add a clear
note near the `[[block]]`/auto-sizing description that the factory auto-sizes
the envelope to contain all blocks, and make the `envelope_half_x_mm`,
`envelope_half_y_mm`, `envelope_z_start_m`, and `envelope_z_end_m` entries
consistent with the geometry tree/test values or explicitly label them as
overridden defaults.
In `@subsystems/MuonShield/src/MuonShieldConfig.cpp`:
- Around line 178-183: The MuonShieldConfig block Z validation only checks the
upstream face, so a block can still extend past the envelope end. Update the
validation in MuonShieldConfig::... around the existing b.start[2] sanity check
to also verify the downstream edge using b.start[2] + b.size[2] against
envEndMm, and throw a similar std::runtime_error when that end lies outside the
envelope.
---
Nitpick comments:
In `@subsystems/MuonShield/src/MuonShieldConfig.cpp`:
- Around line 54-72: The int/float-to-double conversion logic is duplicated
between readNumeric and nodeToNumber in MuonShieldConfig.cpp. Refactor so there
is a single helper that performs the TOML numeric extraction and conversion,
then have the other function delegate to it; keep the existing error text
behavior intact and preserve the current signatures of readNumeric and
nodeToNumber if they’re used elsewhere.
- Around line 162-163: The degree-to-radian constant is duplicated between
MuonShieldConfig.cpp and MuonShieldFactory.cpp under different names, so
centralize it in a shared place such as MuonShieldConfig.h or a common
MuonShield header and update both MuonShieldConfig and MuonShieldFactory to use
the same symbol. Keep the existing kEps local if needed, but remove the
duplicate degToRad/kDegToRad definition so there is a single source of truth for
the conversion constant.
In `@subsystems/MuonShield/test_muonshield.cpp`:
- Around line 86-103: The MuonShield test coverage is missing validation for the
block-level rotation field, so add a new case in test_muonshield.cpp alongside
MuonShieldTaperMakesTrd that configures a non-zero rotation and asserts the
resulting GeoBox or GeoTrd is oriented correctly. Use MuonShieldFactory::build,
the parsed rotation from MuonShieldConfig.cpp, and the block’s start point as
the pivot, then verify the extrinsic X→Y→Z transform is reflected in the child
volume’s shape/orientation.
🪄 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: 943c8557-3dcd-4c43-9a91-07f663968af5
📒 Files selected for processing (10)
src/SHiPGeometry.cppsubsystems/MuonShield/MS.tomlsubsystems/MuonShield/README.mdsubsystems/MuonShield/include/MuonShield/MuonShieldConfig.hsubsystems/MuonShield/include/MuonShield/MuonShieldFactory.hsubsystems/MuonShield/src/MuonShieldConfig.cppsubsystems/MuonShield/src/MuonShieldFactory.cppsubsystems/MuonShield/test_muonshield.cppsubsystems/NeutrinoDetector/README.mdtests/test_consistency.cpp
✅ Files skipped from review due to trivial changes (1)
- subsystems/NeutrinoDetector/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_consistency.cpp
|
In principle everything should be in order now. @Gfrisella the config ( The iron blocks around the SD/SND (will fix name inconsistency in a future PR) are segmented into 4 blocks as it is preferable to give space to the SD. The current version should reflect the geometry changes you made in ShipSoft/FairShip#1334 exactly. |
|
Could you please update the PR title, PR text, and clean up the commit history? |
|
Thanks Matei for the amazing job! |
|
Moreover, would it be possible to allocate the space afterwards? |
|
Will implement, thank you for the comments! |
matclim
changed the title
|
I think we should be clean now @Gfrisella |
Yepppp |
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
subsystems/MuonShield/src/MuonShieldFactory.cpp (1)
41-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing
resolveConfigPathinstead of duplicating the resolution logic.
resolveTomlPathduplicates the same fallback-chain logic already implemented inresolveConfigPath(include/SHiPGeometry/ConfigPath.h). Delegating to the shared helper would eliminate the duplication and automatically pick up the more robustis_regular_filecheck.♻️ Proposed refactor
-#ifndef MS_TOML_DEFAULT_PATH -#define MS_TOML_DEFAULT_PATH "MS.toml" -#endif -#ifndef MS_TOML_INSTALL_PATH -#define MS_TOML_INSTALL_PATH "" -#endif - -static std::string resolveTomlPath(const std::string& path) { - if (std::filesystem::path(path).is_absolute()) - return path; - if (std::filesystem::is_regular_file(path)) - return path; - const std::string srcFallback = MS_TOML_DEFAULT_PATH; - if (std::filesystem::is_regular_file(srcFallback)) - return srcFallback; - const std::string installFallback = MS_TOML_INSTALL_PATH; - if (!installFallback.empty() && std::filesystem::is_regular_file(installFallback)) - return installFallback; - return path; -} +// Keep the MS_TOML_DEFAULT_PATH / MS_TOML_INSTALL_PATH macros for the call site. +static std::string resolveTomlPath(const std::string& path) { + return resolveConfigPath(path, MS_TOML_DEFAULT_PATH, MS_TOML_INSTALL_PATH); +}🤖 Prompt for 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. In `@subsystems/MuonShield/src/MuonShieldFactory.cpp` around lines 41 - 53, Replace the duplicated fallback logic in resolveTomlPath with a call to the shared resolveConfigPath helper from ConfigPath.h, preserving the existing input and return behavior. Ensure the helper’s regular-file validation is used and remove the local filesystem checks and fallback constants that are no longer needed.subsystems/NeutrinoDetector/src/SNDEnvelope.cpp (1)
47-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unknown-key warnings for consistency with
readMuonShieldConfig.
readMuonShieldConfigperforms a first pass that warns about unrecognized top-level keys, catching typos and stale fields.readSNDEnvelopesilently ignores unknown keys — a typo in the optionalrotationfield (e.g.rotaton) would fall back to the default{0,0,0}with no diagnostic, which could be hard to debug in production geometry.♻️ Proposed addition
SNDEnvelope readSNDEnvelope(const std::string& path) { toml::table table; try { table = toml::parse_file(path); } catch (const toml::parse_error& e) { throw std::runtime_error("SNDEnvelope: failed to parse " + path + ": " + std::string(e.description())); } + // Warn about unknown keys (typo / stale-field guard). + static constexpr std::string_view kKnownKeys[] = {"centre", "size", "rotation"}; + for (const auto& [k, _] : table) { + bool known = false; + for (const auto& kk : kKnownKeys) + if (k == kk) { known = true; break; } + if (!known) + std::cerr << "SNDEnvelope: warning: unknown key '" << k << "' in " << path + << " (typo? stale field? — value will be ignored)\n"; + } + SNDEnvelope env;🤖 Prompt for 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. In `@subsystems/NeutrinoDetector/src/SNDEnvelope.cpp` around lines 47 - 64, Add a top-level unknown-key validation pass in readSNDEnvelope, matching readMuonShieldConfig: iterate over the parsed table keys, allow only centre, size, and rotation, and emit warnings for any unrecognized keys before reading the fields.
🤖 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/SHiPGeometry.cpp`:
- Around line 51-68: Register the neutrino detector cavity with
MuonShieldFactory before building the shield: call reserveSpace(...) using the
SND dimensions and placement defined by subsystems/NeutrinoDetector/SD.toml,
centered at 28.95 m. Add this reservation after constructing muonShieldFactory
and before muonShieldFactory.build(), while retaining the existing
embedDaughter(...) call.
In `@subsystems/MuonShield/src/MuonShieldFactory.cpp`:
- Around line 41-53: Replace the std::filesystem::exists checks in
resolveTomlPath with std::filesystem::is_regular_file for the input path and
both fallback paths, preserving the existing absolute-path handling and fallback
order.
- Around line 77-175: Register the SND cavity reservation before constructing
the muon shield: update the SHiP geometry build flow that embeds the neutrino
detector to call MuonShieldFactory::reserveSpace() with the detector’s world
position, dimensions, and rotation before invoking MuonShieldFactory::build().
Ensure this reservation is added before block subtraction in
MuonShieldFactory::build() is reached.
In `@subsystems/NeutrinoDetector/README.md`:
- Around line 51-57: Correct the README description of
MuonShieldFactory::embedDaughter: state that it only nests the pre-built
daughter volume in the muon-shield volume tree, while
MuonShieldFactory::reserveSpace registers and carves the matching air gap from
the shield iron. Preserve the explanation that the SND remains an independent
subsystem.
---
Nitpick comments:
In `@subsystems/MuonShield/src/MuonShieldFactory.cpp`:
- Around line 41-53: Replace the duplicated fallback logic in resolveTomlPath
with a call to the shared resolveConfigPath helper from ConfigPath.h, preserving
the existing input and return behavior. Ensure the helper’s regular-file
validation is used and remove the local filesystem checks and fallback constants
that are no longer needed.
In `@subsystems/NeutrinoDetector/src/SNDEnvelope.cpp`:
- Around line 47-64: Add a top-level unknown-key validation pass in
readSNDEnvelope, matching readMuonShieldConfig: iterate over the parsed table
keys, allow only centre, size, and rotation, and emit warnings for any
unrecognized keys before reading the fields.
🪄 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: a518f180-fba2-496f-be49-b6e0a94c6b92
📒 Files selected for processing (15)
src/SHiPGeometry.cppsubsystems/MuonShield/CMakeLists.txtsubsystems/MuonShield/MS.tomlsubsystems/MuonShield/README.mdsubsystems/MuonShield/include/MuonShield/MuonShieldConfig.hsubsystems/MuonShield/include/MuonShield/MuonShieldFactory.hsubsystems/MuonShield/src/MuonShieldConfig.cppsubsystems/MuonShield/src/MuonShieldFactory.cppsubsystems/MuonShield/test_muonshield.cppsubsystems/NeutrinoDetector/CMakeLists.txtsubsystems/NeutrinoDetector/README.mdsubsystems/NeutrinoDetector/SD.tomlsubsystems/NeutrinoDetector/include/NeutrinoDetector/SNDEnvelope.hsubsystems/NeutrinoDetector/src/SNDEnvelope.cpptests/test_consistency.cpp
| // Build the muon shield, with the neutrino detector embedded inside it. | ||
| // | ||
| // The SND is an independent subsystem, but in volume terms it is a daughter | ||
| // of the muon-shield container: it sits within the shield region (WARM SND | ||
| // slot 26.40–31.50 m → centre 28.95 m). We build it first, then nest it in | ||
| // the shield container. The muon-shield block list must leave this slot | ||
| // free of iron. | ||
| MuonShieldFactory muonShieldFactory(materials); | ||
| GeoPhysVol* muonShield = muonShieldFactory.build(); | ||
| placeChild(world, muonShield, "/SHiP/muon_shield", 2, | ||
| GeoTrf::Translate3D(0.0, 0.0, 16763.3 * mm)); | ||
|
|
||
| // Build and place the Scattering and Neutrino Detector (SND). | ||
| // Z: 26.40 to 31.50 m (WARM muon-shield configuration) → centre 28.95 m. | ||
| // The SND sits within the downstream end of the muon-shield region, so its | ||
| // envelope overlaps the muon-shield container by design (see test_consistency). | ||
| NeutrinoDetectorFactory neutrinoDetectorFactory(materials); | ||
| GeoPhysVol* neutrinoDetector = neutrinoDetectorFactory.build(); | ||
| placeChild(world, neutrinoDetector, "/SHiP/neutrino_detector", 9, | ||
| GeoTrf::Translate3D(0.0, 0.0, 28.95 * m)); | ||
| muonShieldFactory.embedDaughter(neutrinoDetector, 28.95 * 1000.0, "/SHiP/neutrino_detector"); | ||
|
|
||
| // The container is built centred on its own origin, so it is placed at the | ||
| // envelope centre reported by the factory after build(). | ||
| GeoPhysVol* muonShield = muonShieldFactory.build(); | ||
| placeChild(world, muonShield, "/SHiP/muon_shield", 2, | ||
| GeoTrf::Translate3D(0.0, 0.0, muonShieldFactory.centreZ_mm())); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all calls to reserveSpace across the codebase.
rg -nP -C3 '\breservespace\s*\(' --type=cpp -g '!**/build/**'
# Also check if SD.toml is read anywhere outside the provided files.
rg -nP -C3 'SD\.toml' --type=cpp -g '!**/build/**'
# Check the actual MS.toml block list for blocks overlapping the SND Z range (26400-31500 mm).
fd 'MS.toml' --exec sh -c 'echo "=== {} ===" && cat "{}"'Repository: ShipSoft/Geometry
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any reserveSpace implementations/calls.
rg -n --hidden --glob '!**/build/**' '\breserveSpace\s*\(' .
# Find any references to SD.toml.
rg -n --hidden --glob '!**/build/**' 'SD\.toml' .
# Locate geometry-related TOML files.
fd -a 'MS\.toml|SD\.toml|.*\.toml$' .
# Inspect the SHiP geometry source and related factory implementation.
fd -a 'SHiPGeometry.cpp|MuonShieldFactory.*|NeutrinoDetectorFactory.*' src .Repository: ShipSoft/Geometry
Length of output: 3045
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== src/SHiPGeometry.cpp ==='
nl -ba src/SHiPGeometry.cpp | sed -n '1,120p'
echo
echo '=== subsystems/MuonShield/src/MuonShieldFactory.cpp ==='
nl -ba subsystems/MuonShield/src/MuonShieldFactory.cpp | sed -n '1,260p'
echo
echo '=== subsystems/MuonShield/MS.toml ==='
nl -ba subsystems/MuonShield/MS.toml | sed -n '1,220p'
echo
echo '=== subsystems/NeutrinoDetector/SD.toml ==='
nl -ba subsystems/NeutrinoDetector/SD.toml | sed -n '1,220p'Repository: ShipSoft/Geometry
Length of output: 224
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
files = [
"src/SHiPGeometry.cpp",
"subsystems/MuonShield/src/MuonShieldFactory.cpp",
"subsystems/MuonShield/MS.toml",
"subsystems/NeutrinoDetector/SD.toml",
]
for fp in files:
p = Path(fp)
print(f"=== {fp} ===")
if not p.exists():
print("<missing>")
print()
continue
for i, line in enumerate(p.read_text().splitlines(), 1):
print(f"{i:4}: {line}")
print()
PYRepository: ShipSoft/Geometry
Length of output: 18351
Add the SND reservation before building the muon shield in src/SHiPGeometry.cpp:51-62. MuonShieldFactory::build() only subtracts boxes registered via reserveSpace(...); this code embeds the SND but never registers the subsystems/NeutrinoDetector/SD.toml cavity, while subsystems/MuonShield/MS.toml blocks 6–7 already span the 26.4–31.5 m slot. Without that call, the detector overlaps solid iron.
🤖 Prompt for 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.
In `@src/SHiPGeometry.cpp` around lines 51 - 68, Register the neutrino detector
cavity with MuonShieldFactory before building the shield: call reserveSpace(...)
using the SND dimensions and placement defined by
subsystems/NeutrinoDetector/SD.toml, centered at 28.95 m. Add this reservation
after constructing muonShieldFactory and before muonShieldFactory.build(), while
retaining the existing embedDaughter(...) call.
| GeoPhysVol* MuonShieldFactory::build() { | ||
| const GeoMaterial* air = m_materials.requireMaterial("Air"); | ||
|
|
||
| // Overall MuonShieldArea container (Air) | ||
| auto* areaBox = new GeoBox(s_areaHalfX, s_areaHalfY, s_areaHalfZ); | ||
| auto* areaLog = new GeoLogVol("/SHiP/muon_shield", areaBox, air); | ||
| auto* areaPhys = new GeoPhysVol(areaLog); | ||
|
|
||
| // Build and place 6 stations | ||
| for (const StationData& station : k_stations) { | ||
| GeoPhysVol* stationPhys = buildStation(station); | ||
| std::string stationName = "/SHiP/muon_shield/" + std::string(station.name); | ||
| GeoTrf::Transform3D trf = GeoTrf::Translate3D(0.0, 0.0, station.stationZ); | ||
| areaPhys->add(new GeoNameTag(stationName)); | ||
| areaPhys->add(new GeoIdentifierTag(static_cast<int>(&station - &k_stations[0]))); | ||
| areaPhys->add(new GeoTransform(trf)); | ||
| areaPhys->add(stationPhys); | ||
| const MuonShieldConfig cfg = readMuonShieldConfig(resolveTomlPath(m_configPath)); | ||
|
|
||
| GeoMaterial* air = m_materials.requireMaterial("Air"); | ||
| GeoMaterial* absorber = m_materials.requireMaterial(cfg.block_material); | ||
|
|
||
| // Envelope-derived dimensions (mm). | ||
| const double halfZ = 0.5 * cfg.envelopeLengthZ_mm(); | ||
| m_centreZ_mm = cfg.envelopeCentreZ_mm(); | ||
|
|
||
| // Air container spanning the full muon-shield envelope, centred on its own | ||
| // origin (SHiPGeometryBuilder places it at centreZ_mm()). | ||
| auto* containerBox = new GeoBox(cfg.envelope_half_x_mm, cfg.envelope_half_y_mm, halfZ); | ||
| auto* containerLog = new GeoLogVol("/SHiP/muon_shield", containerBox, air); | ||
| auto* containerPhys = new GeoPhysVol(containerLog); | ||
|
|
||
| // Place each iron block. A block is anchored by its upstream (−z) face at | ||
| // its `start`, extends downstream by size[2], is rotated about `start` | ||
| // (extrinsic X→Y→Z), and may taper symmetrically into a GeoTrd. | ||
| int childId = 0; | ||
| for (const auto& b : cfg.blocks) { | ||
| const double halfX = 0.5 * b.size[0]; | ||
| const double halfY = 0.5 * b.size[1]; | ||
| const double halfLenZ = 0.5 * b.size[2]; | ||
|
|
||
| const double farHalfX = halfX + b.size[2] * std::tan(b.taper_deg[0] * kDegToRad); | ||
| const double farHalfY = halfY + b.size[2] * std::tan(b.taper_deg[1] * kDegToRad); | ||
|
|
||
| // Placement transform: shift the solid (centred at its origin) so its | ||
| // upstream face sits at `start`, rotate about `start` (extrinsic | ||
| // X→Y→Z), then translate into place. Container is at m_centreZ_mm in Z. | ||
| const double anchorX = b.start[0]; | ||
| const double anchorY = b.start[1]; | ||
| const double anchorZ = b.start[2] - m_centreZ_mm; | ||
| const GeoTrf::Transform3D rotation = GeoTrf::RotateZ3D(b.rotation_deg[2] * kDegToRad) * | ||
| GeoTrf::RotateY3D(b.rotation_deg[1] * kDegToRad) * | ||
| GeoTrf::RotateX3D(b.rotation_deg[0] * kDegToRad); | ||
| const GeoTrf::Transform3D trf = GeoTrf::Translate3D(anchorX, anchorY, anchorZ) * rotation * | ||
| GeoTrf::Translate3D(0.0, 0.0, halfLenZ); | ||
|
|
||
| GeoShape* shape = nullptr; | ||
| if (b.taper_deg[0] == 0.0 && b.taper_deg[1] == 0.0) | ||
| shape = new GeoBox(halfX, halfY, halfLenZ); | ||
| else | ||
| shape = new GeoTrd(halfX, farHalfX, halfY, farHalfY, halfLenZ); | ||
|
|
||
| // Carve out any reserved box that intersects this magnet (A - B). The | ||
| // reserved box, given in world coords, is expressed in the block's own | ||
| // (shape) frame before subtracting. | ||
| const GeoTrf::Vector3D blockCentre = trf.translation(); | ||
| const double blockMaxHX = std::max(halfX, farHalfX); | ||
| const double blockMaxHY = std::max(halfY, farHalfY); | ||
| for (const auto& r : m_reservations) { | ||
| const double rcx = r.centre_mm[0]; | ||
| const double rcy = r.centre_mm[1]; | ||
| const double rcz = r.centre_mm[2] - m_centreZ_mm; | ||
| const double rhx = 0.5 * r.size_mm[0]; | ||
| const double rhy = 0.5 * r.size_mm[1]; | ||
| const double rhz = 0.5 * r.size_mm[2]; | ||
| const bool intersects = std::abs(blockCentre.x() - rcx) < blockMaxHX + rhx && | ||
| std::abs(blockCentre.y() - rcy) < blockMaxHY + rhy && | ||
| std::abs(blockCentre.z() - rcz) < halfLenZ + rhz; | ||
| if (!intersects) | ||
| continue; | ||
| const GeoTrf::Transform3D resRot = GeoTrf::RotateZ3D(r.rotation_deg[2] * kDegToRad) * | ||
| GeoTrf::RotateY3D(r.rotation_deg[1] * kDegToRad) * | ||
| GeoTrf::RotateX3D(r.rotation_deg[0] * kDegToRad); | ||
| const GeoTrf::Transform3D resInBlock = | ||
| trf.inverse() * GeoTrf::Translate3D(rcx, rcy, rcz) * resRot; | ||
| auto* voidBox = new GeoBox(rhx, rhy, rhz); | ||
| shape = new GeoShapeSubtraction(shape, new GeoShapeShift(voidBox, resInBlock)); | ||
| } | ||
|
|
||
| const std::string blockName = "/SHiP/muon_shield/block_" + std::to_string(childId); | ||
| auto* blockLog = new GeoLogVol(blockName, shape, absorber); | ||
| auto* blockPhys = new GeoPhysVol(blockLog); | ||
|
|
||
| containerPhys->add(new GeoNameTag(blockName)); | ||
| containerPhys->add(new GeoIdentifierTag(childId++)); | ||
| containerPhys->add(new GeoTransform(trf)); | ||
| containerPhys->add(blockPhys); | ||
| } | ||
|
|
||
| // Place the embedded daughters inside the container, keeping them as | ||
| // independent, named subsystems. (Callers must not list iron blocks over a | ||
| // daughter's footprint — the block list is the sole source of iron.) | ||
| for (const auto& d : m_daughters) { | ||
| const double localZ = d.worldCentreZ_mm - m_centreZ_mm; | ||
| if (std::abs(localZ) > halfZ) | ||
| throw std::runtime_error("MuonShieldFactory: embedded daughter '" + d.name + | ||
| "' lies outside the shield envelope in Z"); | ||
| containerPhys->add(new GeoNameTag(d.name)); | ||
| containerPhys->add(new GeoIdentifierTag(childId++)); | ||
| containerPhys->add(new GeoTransform(GeoTrf::Translate3D(0.0, 0.0, localZ))); | ||
| containerPhys->add(d.volume); | ||
| } | ||
|
|
||
| return areaPhys; | ||
| return containerPhys; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all calls to reserveSpace in the codebase
rg -nP '\breserveSpace\s*\(' --type=cpp -C3Repository: ShipSoft/Geometry
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '## reserveSpace references (all files)'
rg -n -S '\breserveSpace\s*\(' .
echo
echo '## embedDaughter references'
rg -n -S '\bembedDaughter\s*\(' .
echo
echo '## MuonShieldFactory references'
rg -n -S '\bMuonShieldFactory\b' .
echo
echo '## SHiPGeometryBuilder references'
rg -n -S '\bSHiPGeometryBuilder\b' .Repository: ShipSoft/Geometry
Length of output: 6295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '## src/SHiPGeometry.cpp'
sed -n '1,180p' src/SHiPGeometry.cpp
echo
echo '## subsystems/MuonShield/README.md around reserveSpace'
sed -n '80,110p' subsystems/MuonShield/README.md
echo
echo '## subsystems/MuonShield/test_muonshield.cpp around reserveSpace/embedDaughter'
sed -n '70,170p' subsystems/MuonShield/test_muonshield.cppRepository: ShipSoft/Geometry
Length of output: 11843
Call reserveSpace() before building the muon shield src/SHiPGeometry.cpp embeds the neutrino detector, but never registers the SND cavity reservation, so the subtraction in MuonShieldFactory::build() never runs and the detector can overlap iron.
🤖 Prompt for 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.
In `@subsystems/MuonShield/src/MuonShieldFactory.cpp` around lines 77 - 175,
Register the SND cavity reservation before constructing the muon shield: update
the SHiP geometry build flow that embeds the neutrino detector to call
MuonShieldFactory::reserveSpace() with the detector’s world position,
dimensions, and rotation before invoking MuonShieldFactory::build(). Ensure this
reservation is added before block subtraction in MuonShieldFactory::build() is
reached.
|
@olantwin I think we are good now |
I'm review Monday. Could you please clean up the commit history? |
|
For the config path resolution, please reuse the version we already have, we don't need to reimplement this for every detector. |
Replace the GDML-derived station stub with a configurable block list driven by MS.toml (MuonShieldConfig / readMuonShieldConfig, toml++), mirroring the calorimeter's calo.toml pattern. Each block is positioned by its upstream face and may be rotated or tapered (GeoBox, or GeoTrd when tapered). The shipped MS.toml reproduces the FairShip TRY_2026 magnets (FairShip PR #1334) as a solid-block approximation: each magnet is one iron block sized to its FairShip outer envelope, dropping the aperture, yoke shape and field (field is handled elsewhere). The FairShip absorber is the magnetised hadron stopper and stays in the Target subsystem. Place the container at the envelope centre reported by the factory instead of the hard-coded GDML z, and nest the neutrino detector inside the shield container as an embedded daughter while keeping it an independent subsystem. BREAKING CHANGE: MuonShieldFactory's public API and the muon-shield geometry and placement have changed; the old station/piece structs are removed.
Define the muon shield and the neutrino detector independently and carve the SND cavity out of the iron at build time. MuonShieldFactory::reserveSpace() registers a box; build() subtracts it (GeoShapeSubtraction) from every magnet it intersects and leaves the rest untouched, so which magnets are carved follows from the reservation rather than a hard-coded list. The reservation envelope is declared in NeutrinoDetector/SD.toml, parsed by SNDEnvelope via the shared resolveConfigPath helper, and wired in by SHiPGeometryBuilder before the shield is built.
|
Worth noting that commit 084c088 will likely not compile standalone as a result of the rebase. I'm not sure whether this is worth working around. |
3 participants
Replace the GDML-derived 6-station stub with a configurable iron-block muon
shield. The blocks reproduce the FairShip TRY_2026 magnet layout (PR #1334) as a
solid-block approximation: each magnet is one iron block sized to its FairShip
outer envelope, dropping the aperture, yoke shape and field (field is handled
elsewhere). Magnets may be positioned, rotated, and tapered (GeoBox, or GeoTrd
when tapered). The FairShip absorber (the magnetised hadron stopper) stays in the
Target subsystem, so the shield is the 7 downstream magnets.
Geometry is driven by MS.toml via MuonShieldConfig / readMuonShieldConfig
(toml++), mirroring the calorimeter's calo.toml pattern: unknown-key warnings,
size/taper/envelope validation, and CWD / source-tree / install path resolution.
CMake stages and installs MS.toml and bakes the fallback paths. The container is
placed at the envelope centre reported by the factory rather than a hard-coded
GDML z position.
The muon shield and the Scattering & Neutrino Detector are defined independently.
The SND reservation envelope is declared in NeutrinoDetector/SD.toml (parsed by
SNDEnvelope) and carved from the shield iron at build time via Boolean
subtraction (MuonShieldFactory::reserveSpace -> GeoShapeSubtraction on every
magnet the box intersects); the SND is then embedded as a daughter of the shield
container, sitting in the resulting cavity. Which magnets are carved follows from
the reservation position, not a hard-coded list.
BREAKING CHANGE: MuonShieldFactory's public API and the muon-shield geometry and
placement have changed; the old station/piece structs are removed.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation