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 5 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 geometry is now read from ChangesConfigured MuonShield geometry and nested detector
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SHiPGeometryBuilder
participant SNDEnvelope
participant MuonShieldFactory
participant NeutrinoDetector
SHiPGeometryBuilder->>SNDEnvelope: read SD.toml
SHiPGeometryBuilder->>MuonShieldFactory: reserveSpace(envelope)
SHiPGeometryBuilder->>NeutrinoDetector: build detector
SHiPGeometryBuilder->>MuonShieldFactory: embedDaughter(detector)
MuonShieldFactory-->>SHiPGeometryBuilder: built shield 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
|
@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. |
olantwin
left a comment
olantwin
left a comment
There was a problem hiding this comment.
Mostly questions to understand better how everything works.
| 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"); |
There was a problem hiding this comment.
This duplicates the number from the SD toml. Does the embed create the gap in the iron?
There was a problem hiding this comment.
No, embedDaughter only nests the pre-built volume in the shield's tree; it doesn't touch the iron. The gap is carved by reserveSpace → GeoShapeSubtraction in build(). That call was missing, which is the bug in comment 2. Both the reservation and the embed now derive from the same SNDEnvelope read from SD.toml, so the 28.95 literal is gone and the duplicate world-placement is removed.
| // 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.
I'd concur with coderabbit that it seems that the space is never reserved. Should the reserveSpace function be called first?
There was a problem hiding this comment.
Correct, thank you, it wasn't being called at all. The builder now does reserveSpace(snd.centre_mm, snd.size_mm, snd.rotation_deg) before build(). OverlapCheck confirms the SND now sits in a carved cavity rather than solid iron.
| {"/SHiP/muon_shield", 18310.0, 500.0}, | ||
| // The neutrino detector is nested inside the muon shield (see | ||
| // MuonShieldFactory::embedDaughter), not a direct world child, so it is | ||
| // not among collectSubsystems(world). |
There was a problem hiding this comment.
Can we test this in another way? Maybe check the placement within the muon shield?
There was a problem hiding this comment.
Done, added ConsistencyTest.NeutrinoDetectorNestedInMuonShield, which locates /SHiP/muon_shield among the world's children, asserts /SHiP/neutrino_detector is among its children, and checks it's not a direct world child. So it's a positive assertion of the nesting rather than an omission.
| throw std::runtime_error("MuonShieldConfig: " + where + | ||
| " has a taper that collapses its downstream face"); | ||
|
|
||
| // The block must sit within the envelope in Z (light sanity check; |
There was a problem hiding this comment.
Can we also check the envelope in x/y? For collisions with TCC8 this might be relevant.
There was a problem hiding this comment.
Added. Each block's transverse extent (|start| + half-size, taper-aware) is now checked against envelope_half_x/y_mm and throws if it pokes out. Good point regarding TCC8.
| 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 && |
There was a problem hiding this comment.
The half-widths here are the unrotated ones, right? In that case, the actual bounding box might be different if rotations are included.
There was a problem hiding this comment.
Right. The intersection test now computes the block's true axis-aligned bounding box in the container frame by transforming its 8 corners through the placement transform, so it's correct under rotation. (The subtraction geometry itself was always exact — this only governs whether it's applied, so no change to the current geometry, but it's correct now for rotated blocks.)
| // 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) |
There was a problem hiding this comment.
Again, we should probably also check x and y.
There was a problem hiding this comment.
embedDaughter takes only a Z centre, so daughters are placed on the beam axis (x = y = 0) by construction, the Z fit is the only bound to check here. I've documented that in the code. If we ever support off-axis daughters, a transverse-extent check against the envelope (from the daughter's bounding box) should be added, happy to do it then, or now if you'd prefer. I left it out to avoid special-casing GeoBox.
There was a problem hiding this comment.
Ok, since it's documented we can leave it for now.
There was a problem hiding this comment.
It would be good if we can enforce (by design ideally, otherwise via error messages/documentation) that the different operations happen in the right order.
E.g. reserving space or embedding daughter after build() has not effect, it should not fail silently.
Maybe we can read the config in the constructor so that certain variables can not be initialized? Or check that a config is loaded before certain operations?
There was a problem hiding this comment.
Added an m_built guard: reserveSpace/embedDaughter after build(), and a second build(), now throw with a clear message instead of no-oping. On reading the config in the constructor: I like the direction, but it moves file I/O and its error handling into construction, which changes when/where failures surface; I'd prefer to keep the guard for now and do constructor-loading as a separate change if you feel it's worth it.
| 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 + |
There was a problem hiding this comment.
Added (and for std::max, which was also being used transitively).
| CHECK_THAT(ms->getXToChildVol(0).translation().z(), | ||
| Catch::Matchers::WithinAbs(blockCentreLocalZ, 1e-3)); | ||
|
|
||
| // Magnet 6 (index 5) widens in Y → GeoTrd. |
There was a problem hiding this comment.
Not anymore now that M6 is no longer tapered. It's been updated accordingly so the check looks for a regular GeoBox.
| ```text | ||
| muon_shield (Air envelope, 3520 × 2640 × 27540 mm) centre z = 18.31 m | ||
| ├─ block_0 magnet 1 (Iron box) | ||
| ├─ … magnets 2–5 (Iron boxes; magnet 6 is a GeoTrd, widens in Y) |
There was a problem hiding this comment.
Is magnet 6 still tapered?
There was a problem hiding this comment.
No longer FairShip #1342 (fix: SND magnet configuration) made magnet 6's upstream and downstream faces equal, so it's a plain box now, not a GeoTrd. Updated both the config and the assertion; taper so GeoTrd stays covered by MuonShieldTaperMakesTrd.
|
Thanks for the thorough review, comments 1 and 2 caught a real bug: reserveSpace() was never called, so no cavity was carved and the SND was overlapping solid iron. It is fixed, plus the hardening you flagged. All building with tests green (incl. OverlapCheck, which now genuinely exercises the carved cavity). |
|
Thanks for the responses. I'd suggest adding additional commits now addressing them, we can then squash things later. Please leave the threads unresolved for now, I'll resolve them while I review the new version. |
|
Done, I thought I had pushed beforehand but apparently not. |
|
Thanks, I'll review ASAP. |
|
While I review, could you please also ask coderabbit for a full review? |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 59 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
subsystems/MuonShield/src/MuonShieldConfig.cpp (1)
174-207: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider validating that blocks don't overlap each other in Z.
Validation covers per-block bounds (size, taper, envelope Z/X/Y) but never checks that two blocks' Z ranges don't overlap. A future MS.toml edit that accidentally overlaps two blocks would only surface later via
OverlapCheck/geometry tests instead of failing fast here, right next to the other geometric sanity checks.♻️ Proposed addition (after the existing per-block validation loop)
+ // Blocks must not overlap each other in Z (unrotated check, same caveat as above). + for (std::size_t i = 0; i < cfg.blocks.size(); ++i) { + for (std::size_t j = i + 1; j < cfg.blocks.size(); ++j) { + const auto& a = cfg.blocks[i]; + const auto& b = cfg.blocks[j]; + const bool overlap = a.start[2] < b.start[2] + b.size[2] - kEps && + b.start[2] < a.start[2] + a.size[2] - kEps; + if (overlap) + throw std::runtime_error("MuonShieldConfig: blocks " + std::to_string(i) + " and " + + std::to_string(j) + " overlap in Z in " + 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/MuonShieldConfig.cpp` around lines 174 - 207, Extend the validation around the per-block checks to detect overlapping Z intervals between distinct blocks. Compare each block’s upstream and downstream Z bounds against every later block, and throw a descriptive runtime error identifying both block indices when their ranges overlap, while allowing merely touching boundaries.subsystems/NeutrinoDetector/SD.toml (1)
14-15: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConfirm whether magnet 7 should remain uncarved before accepting this reservation envelope.
centre.z = 28950withsize.z = 5100produces a reservation range of[26400, 31500], which overlaps magnet 6 by 1440 mm and magnet 7 by 3470 mm, but does not overlap magnet 5. If the intended SND cavity is still “magnets 5 and 6 only,” the reservation needs to move upstream rather than carve magnet 7.🤖 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/SD.toml` around lines 14 - 15, Confirm the intended SND cavity magnet coverage for the reservation envelope defined by centre and size in SD.toml. If it must cover only magnets 5 and 6, adjust centre.z and/or size.z so the resulting z-range excludes magnet 7 while retaining the required magnet 5–6 coverage; otherwise explicitly preserve the current envelope for magnet 7.
🤖 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/CMakeLists.txt`:
- Around line 32-38: Update MS_TOML_INSTALL_PATH in
subsystems/MuonShield/CMakeLists.txt (lines 32-38) to use
CMAKE_INSTALL_FULL_DATADIR. Apply the same change to SD_TOML_INSTALL_PATH in
subsystems/NeutrinoDetector/CMakeLists.txt (lines 38-45), preserving the
existing SHiPGeometry subdirectory and TOML filenames.
In `@subsystems/MuonShield/src/MuonShieldFactory.cpp`:
- Around line 127-160: Update the bounding-box validation around the block
construction to account for both rotation gaps. Compute each reservation’s
world-axis-aligned bounds from all corners transformed by its rotation before
the intersection test, and use those bounds instead of unrotated rhx/rhy/rhz in
the checks preceding GeoShapeSubtraction. Also validate bLoX..bHiZ against the
configured envelope half-extents and halfZ, rejecting any rotated block whose
AABB exceeds the Air container.
---
Nitpick comments:
In `@subsystems/MuonShield/src/MuonShieldConfig.cpp`:
- Around line 174-207: Extend the validation around the per-block checks to
detect overlapping Z intervals between distinct blocks. Compare each block’s
upstream and downstream Z bounds against every later block, and throw a
descriptive runtime error identifying both block indices when their ranges
overlap, while allowing merely touching boundaries.
In `@subsystems/NeutrinoDetector/SD.toml`:
- Around line 14-15: Confirm the intended SND cavity magnet coverage for the
reservation envelope defined by centre and size in SD.toml. If it must cover
only magnets 5 and 6, adjust centre.z and/or size.z so the resulting z-range
excludes magnet 7 while retaining the required magnet 5–6 coverage; otherwise
explicitly preserve the current envelope for magnet 7.
🪄 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 Plus
Run ID: 1ed3be33-b454-49d3-a20e-3faabddbbef8
📒 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
@matclim these two nitpicks seem worth addressing. |
|
Update underway, have to finish with another PR first |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
subsystems/MuonShield/src/MuonShieldConfig.cpp (2)
174-207: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBlocks aren't checked for mutual Z overlap.
Only the envelope bounds are validated per block; nothing detects two blocks whose
[start_z, start_z+size_z]ranges overlap. This is the outstanding nitpick called out in the latest review (per PR objectives) and isn't addressed in this version.🛡️ Suggested overlap check (after the per-block loop)
} + + // Distinct blocks must not overlap in Z (order-independent check). + std::vector<std::size_t> order(cfg.blocks.size()); + for (std::size_t i = 0; i < order.size(); ++i) order[i] = i; + std::sort(order.begin(), order.end(), + [&](std::size_t a, std::size_t b) { return cfg.blocks[a].start[2] < cfg.blocks[b].start[2]; }); + for (std::size_t k = 1; k < order.size(); ++k) { + const auto& prev = cfg.blocks[order[k - 1]]; + const auto& cur = cfg.blocks[order[k]]; + if (prev.start[2] + prev.size[2] > cur.start[2] + kEps) + throw std::runtime_error("MuonShieldConfig: block " + std::to_string(order[k - 1]) + + " overlaps block " + std::to_string(order[k]) + " in Z in " + path); + } return cfg;🤖 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 174 - 207, Add mutual Z-overlap validation after the per-block checks in the configuration validation routine. Compare each pair of blocks using their [start[2], start[2] + size[2]] intervals, reject any overlapping ranges with a runtime_error identifying both block indices, and allow merely touching endpoints.
119-158: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnknown-key warning only covers top-level keys, not per-block keys.
Typos inside
[[block]]tables (e.g.rotatoninstead ofrotation) are silently ignored — the intended "typo? stale field?" protection doesn't reach the part of the schema most likely to be hand-edited per magnet.🔍 Suggested extension
+static constexpr std::array kKnownBlockKeys = {"rotation"sv, "size"sv, "start"sv, "taper"sv}; + // First pass: warn about unknown keys. for (const auto& [k, _] : table) { if (!std::ranges::binary_search(kKnownKeys, std::string_view{k})) { std::cerr << "MuonShieldConfig: warning: unknown key '" << k << "' in " << path << " (typo? stale field? — value will be ignored)\n"; } } +// ... inside the block loop, after `const toml::table* bt = elem.as_table();` +for (const auto& [k, _] : *bt) { + if (!std::ranges::binary_search(kKnownBlockKeys, std::string_view{k})) + std::cerr << "MuonShieldConfig: warning: unknown block key '" << k << "' in " << path + << "\n"; +}🤖 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 119 - 158, Extend unknown-key validation to each [[block]] table in the block-parsing loop, using the block schema keys start, size, rotation, and taper. Emit the same warning format for unrecognized per-block keys before reading the known fields, while preserving the existing parsing and validation behavior.subsystems/MuonShield/include/MuonShield/MuonShieldConfig.h (1)
76-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring is out of date vs. the validation actually implemented.
readMuonShieldConfig()'s@throwsdoc only mentions non-positive/inverted Z envelope, non-positive block size, taper collapse, and upstream-face-out-of-envelope.MuonShieldConfig.cppalso throws for the downstream face extending past the envelope end and for blocks extending beyond the envelope in x/y — neither is mentioned here.📝 Proposed doc update
* `@throws` std::runtime_error if the file cannot be opened, contains malformed * TOML, has a non-positive or inverted Z envelope, or a block with a * non-positive size, a taper that collapses the far face, or an - * upstream face outside the envelope in Z. + * upstream or downstream face outside the envelope in Z, or a + * transverse (x/y) extent exceeding the envelope half-widths. */🤖 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 76 - 88, Update the `@throws` documentation for readMuonShieldConfig to include validation failures when a downstream face exceeds the envelope end and when a block extends beyond the envelope in x or y. Keep the existing documented failure cases unchanged and do not modify implementation behavior.subsystems/NeutrinoDetector/CMakeLists.txt (1)
11-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame TOML staging/install/compile-definitions boilerplate duplicated from MuonShield's
CMakeLists.txt.See the consolidated comment for a suggested shared-function extraction covering both files (and the existing Calorimeter copy).
🤖 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/CMakeLists.txt` around lines 11 - 45, Extract the duplicated TOML dependency, SD.toml staging/install, and resolveConfigPath() compile-definition setup from this CMakeLists.txt and MuonShield’s CMakeLists.txt into the shared CMake helper proposed by the consolidated review comment. Replace the local boilerplate with the helper invocation, preserving the existing target, source file, install destination, and generated-path behavior; also reuse that helper for the existing Calorimeter equivalent.subsystems/MuonShield/CMakeLists.txt (1)
10-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTOML staging/install/compile-definitions boilerplate is duplicated across subsystems.
This block (find_package, private BUILD_INTERFACE link, dual
configure_filestaging,install, and fallbacktarget_compile_definitions) is essentially copy-pasted insubsystems/NeutrinoDetector/CMakeLists.txt(and, per the comment on line 8, already exists a third time in Calorimeter'sCMakeLists.txt). The earlierCMAKE_INSTALL_FULL_DATADIRfix had to be applied to two files separately, which is direct evidence of the duplication cost. See the consolidated comment below for a suggested shared-function extraction.🤖 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/CMakeLists.txt` around lines 10 - 43, Extract the repeated TOML dependency, staging, installation, and fallback-definition setup from the MuonShield CMake configuration into the shared helper suggested by the consolidated review comment. Update MuonShield to invoke that helper with its target and MS.toml source, preserving the existing private tomlplusplus link, both staging destinations, install destination, and MS_TOML_DEFAULT_PATH/MS_TOML_INSTALL_PATH definitions; apply the same helper to the corresponding NeutrinoDetector and Calorimeter configurations.subsystems/NeutrinoDetector/src/SNDEnvelope.cpp (1)
29-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate TOML vec3-parsing helpers vs.
MuonShieldConfig.cpp.
toNumber/readVec3reimplement the same required/fallback array-parsing pattern asnodeToNumber/readFixedArrayinMuonShieldConfig.cpp(the code comment on Line 77 even says "like readMuonShieldConfig"). Consider factoring this into a shared TOML helper (e.g. a small header underinclude/SHiPGeometry/) used by bothMuonShieldConfig.cppand this file, so future validation changes don't need to be kept in sync manually in two places.🤖 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 29 - 56, Factor the duplicated TOML numeric and fixed-length array parsing from toNumber/readVec3 and MuonShieldConfig.cpp into a shared helper under include/SHiPGeometry/. Update both callers to use the shared helper while preserving required-field handling, fallbacks, three-element validation, numeric conversion, and existing error behavior.subsystems/MuonShield/src/MuonShieldFactory.cpp (1)
92-150: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo validation that distinct blocks don't overlap each other in Z.
The block loop validates a single block's rotation-aware AABB against the envelope (Lines 142-150), but there's still no check that two different blocks in
cfg.blocksdon't overlap each other in Z. Per the PR discussion, this was flagged as an outstanding nitpick and is still pending. SincebLoZ/bHiZare already computed per block here, tracking previously-seen Z ranges and comparing would be a fairly cheap addition (either here or inreadMuonShieldConfig's existing validation pass).🤖 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 92 - 150, The block loop validates each block against the envelope but does not reject overlaps between distinct blocks along Z. Track each computed block Z interval from bLoZ and bHiZ, compare it with previously accepted intervals, and throw a configuration error when intervals overlap; retain the existing envelope validation and allow merely touching boundaries.subsystems/MuonShield/test_muonshield.cpp (1)
96-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing regression coverage for the rotation-aware AABB fixes.
No test exercises the two rotation-aware checks added in
MuonShieldFactory::build(): (a) a rotated block whose AABB exceeds the envelope should throw, and (b) a rotatedreserveSpace(...)box correctly intersects/carves a block. Both paths were previously buggy (per past review) before being fixed; a couple of smallwriteTempToml-based cases mirroringMuonShieldTaperMakesTrd/MuonShieldReservationCarvesIronwould guard against regressions here.🤖 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 96 - 138, Add regression tests in the MuonShield test suite covering both rotation-aware checks in MuonShieldFactory::build(): verify a rotated block whose computed AABB exceeds the envelope throws, and verify a rotated reserveSpace(...) box intersects and carves the target block as expected. Use small writeTempToml-based cases consistent with MuonShieldTaperMakesTrd and MuonShieldReservationCarvesIron, asserting the exception and resulting geometry respectively.
🤖 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 116-125: Clarify the README’s description of the shipped default
geometry to state that it is a FairShip outer-envelope approximation, not the
finalized engineering layout. Update the status/TODO wording around the default
blocks to consistently reflect this approximation and their pending replacement
with the coordinator-supplied layout.
---
Nitpick comments:
In `@subsystems/MuonShield/CMakeLists.txt`:
- Around line 10-43: Extract the repeated TOML dependency, staging,
installation, and fallback-definition setup from the MuonShield CMake
configuration into the shared helper suggested by the consolidated review
comment. Update MuonShield to invoke that helper with its target and MS.toml
source, preserving the existing private tomlplusplus link, both staging
destinations, install destination, and MS_TOML_DEFAULT_PATH/MS_TOML_INSTALL_PATH
definitions; apply the same helper to the corresponding NeutrinoDetector and
Calorimeter configurations.
In `@subsystems/MuonShield/include/MuonShield/MuonShieldConfig.h`:
- Around line 76-88: Update the `@throws` documentation for readMuonShieldConfig
to include validation failures when a downstream face exceeds the envelope end
and when a block extends beyond the envelope in x or y. Keep the existing
documented failure cases unchanged and do not modify implementation behavior.
In `@subsystems/MuonShield/src/MuonShieldConfig.cpp`:
- Around line 174-207: Add mutual Z-overlap validation after the per-block
checks in the configuration validation routine. Compare each pair of blocks
using their [start[2], start[2] + size[2]] intervals, reject any overlapping
ranges with a runtime_error identifying both block indices, and allow merely
touching endpoints.
- Around line 119-158: Extend unknown-key validation to each [[block]] table in
the block-parsing loop, using the block schema keys start, size, rotation, and
taper. Emit the same warning format for unrecognized per-block keys before
reading the known fields, while preserving the existing parsing and validation
behavior.
In `@subsystems/MuonShield/src/MuonShieldFactory.cpp`:
- Around line 92-150: The block loop validates each block against the envelope
but does not reject overlaps between distinct blocks along Z. Track each
computed block Z interval from bLoZ and bHiZ, compare it with previously
accepted intervals, and throw a configuration error when intervals overlap;
retain the existing envelope validation and allow merely touching boundaries.
In `@subsystems/MuonShield/test_muonshield.cpp`:
- Around line 96-138: Add regression tests in the MuonShield test suite covering
both rotation-aware checks in MuonShieldFactory::build(): verify a rotated block
whose computed AABB exceeds the envelope throws, and verify a rotated
reserveSpace(...) box intersects and carves the target block as expected. Use
small writeTempToml-based cases consistent with MuonShieldTaperMakesTrd and
MuonShieldReservationCarvesIron, asserting the exception and resulting geometry
respectively.
In `@subsystems/NeutrinoDetector/CMakeLists.txt`:
- Around line 11-45: Extract the duplicated TOML dependency, SD.toml
staging/install, and resolveConfigPath() compile-definition setup from this
CMakeLists.txt and MuonShield’s CMakeLists.txt into the shared CMake helper
proposed by the consolidated review comment. Replace the local boilerplate with
the helper invocation, preserving the existing target, source file, install
destination, and generated-path behavior; also reuse that helper for the
existing Calorimeter equivalent.
In `@subsystems/NeutrinoDetector/src/SNDEnvelope.cpp`:
- Around line 29-56: Factor the duplicated TOML numeric and fixed-length array
parsing from toNumber/readVec3 and MuonShieldConfig.cpp into a shared helper
under include/SHiPGeometry/. Update both callers to use the shared helper while
preserving required-field handling, fallbacks, three-element validation, numeric
conversion, and existing error behavior.
🪄 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 Plus
Run ID: 8d9e83cc-eaaa-46f9-90b1-6fe59ac7934e
📒 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
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 37 minutes. |
Add cmake/ShipAddTomlConfig.cmake (toml++ link, staging, install, and *_TOML_DEFAULT_PATH/*_TOML_INSTALL_PATH defines) and include/SHiPGeometry/TomlConfig.h (shared asDouble + readNumericArray). MuonShield, NeutrinoDetector, and Calorimeter now call these instead of each carrying its own copy of the CMake boilerplate and TOML numeric / fixed-array parsing. Error-message wording for array parsing is now uniform across subsystems; the throw conditions are unchanged.
Add cmake/ShipAddTomlConfig.cmake (toml++ link, staging, install, and *_TOML_DEFAULT_PATH/*_TOML_INSTALL_PATH defines) and include/SHiPGeometry/TomlConfig.h (shared asDouble + readNumericArray). MuonShield, NeutrinoDetector, and Calorimeter now call these instead of each carrying its own copy of the CMake boilerplate and TOML numeric / fixed-array parsing. Error-message wording for array parsing is now uniform across subsystems; the throw conditions are unchanged.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/SHiPGeometry.cpp (1)
58-62: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRegister the SD.toml reservation before building the shield. The builder embeds the SND but never calls
reserveSpace(), leavingm_reservationsempty and the intersecting iron solid. Read the SND envelope once and use its centre, size, and rotation for both reservation and embedding.
src/SHiPGeometry.cpp#L58-L62: callreserveSpace(...)from the parsed SND envelope beforeMuonShieldFactory::build().subsystems/MuonShield/README.md#L24-L42: retain the carving description only after the builder wiring exists.subsystems/NeutrinoDetector/README.md#L51-L59: retain thereserveSpaceclaim only after the builder wiring exists.🤖 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 58 - 62, Update src/SHiPGeometry.cpp lines 58-62 to parse the SND envelope once, then pass its centre, size, and rotation to reserveSpace(...) before MuonShieldFactory::build(), reusing the same values for embedDaughter(...). Retain the carving description in subsystems/MuonShield/README.md lines 24-42 and the reserveSpace claim in subsystems/NeutrinoDetector/README.md lines 51-59 only after the builder wiring is implemented.subsystems/MuonShield/src/MuonShieldFactory.cpp (1)
110-131: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse transformed AABBs for the carve decision.
The intersection check uses unrotated block and reservation half-extents, even though both support rotation. A rotated reservation or block can be classified as non-intersecting, skipping the subtraction and leaving iron in the intended cavity. Compute each transformed box’s world AABB from its corners; also reject a rotated block whose AABB exceeds the Air envelope.
🤖 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 110 - 131, Update the carve-intersection logic around the reservation loop to compute world-axis-aligned bounding boxes from all eight transformed corners for both the block and each reservation, including their rotations. Use those AABBs for overlap testing, and reject rotated blocks whose transformed AABB exceeds the Air envelope before applying subtraction; preserve the existing transformed reservation subtraction via resInBlock.
🧹 Nitpick comments (2)
subsystems/MuonShield/test_muonshield.cpp (1)
89-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that magnet 6 is carved too.
The SND reservation spans magnets 6 and 7, but this test only verifies the last magnet’s subtraction shape. Assert
getChildVol(5)is also aGeoShapeSubtractionso a partial cavity regression is caught.Proposed test addition
CHECK(dynamic_cast<const GeoBox*>(ms->getChildVol(0)->getLogVol()->getShape()) != nullptr); + CHECK(dynamic_cast<const GeoShapeSubtraction*>( + ms->getChildVol(5)->getLogVol()->getShape()) != nullptr); // The last magnet intersects the reservation → its shape is a subtraction. CHECK(dynamic_cast<const GeoShapeSubtraction*>(ms->getChildVol(6)->getLogVol()->getShape()) != nullptr);🤖 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 89 - 93, Extend the test assertions after the existing magnet 1 check to verify that ms->getChildVol(5) has a GeoShapeSubtraction shape, while preserving the current assertion for ms->getChildVol(6). This ensures both reserved magnets are confirmed as carved.subsystems/MuonShield/src/MuonShieldFactory.cpp (1)
81-81: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReject overlapping block Z-ranges before placing siblings.
The current validation only checks each block against the envelope; it never compares blocks with each other. A TOML typo can therefore create overlapping iron children. Validate pairwise Z intervals (using transformed bounds when rotation is enabled) and add a rejection test.
🤖 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` at line 81, Extend validation in MuonShieldFactory’s block-processing flow to compare every pair of sibling block Z-intervals before placement, using transformed bounds when rotation is enabled. Reject configurations with any overlap while preserving the existing envelope validation, and add a test covering overlapping block ranges.
🤖 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 29-33: Update the magnet-6 description in
subsystems/MuonShield/README.md lines 29-33 to identify it as an iron box rather
than a widening GeoTrd, and update the corresponding test comment in
subsystems/MuonShield/test_muonshield.cpp lines 74-75 to match the GeoBox
assertion; no code behavior changes are needed.
In `@subsystems/MuonShield/src/MuonShieldConfig.cpp`:
- Around line 139-161: Add the missing transverse envelope validation inside the
block loop alongside the existing Z checks. In the validation flow using
b.start, b.size, and taper-aware downstream dimensions, compute each block’s
maximum absolute x/y extent across its tapered span and reject blocks exceeding
envelope_half_x_mm or envelope_half_y_mm, including the block location and
offending extent in the runtime error.
- Around line 139-161: Validate distinct block Z intervals after the per-block
checks in the configuration validation flow, using each block’s start[2] and
size[2]. Detect any overlapping intervals, including unsorted blocks, and throw
a runtime_error identifying both conflicting block indices and the configuration
path; retain the existing envelope and taper validation unchanged.
In `@subsystems/MuonShield/src/MuonShieldFactory.cpp`:
- Around line 44-52: Restore the m_built guard in
MuonShieldFactory::embedDaughter and reserveSpace so they throw before accepting
registrations after geometry has been built. Add the same precondition to
build() to reject repeated builds, preserving the existing hierarchy creation
only for the first build.
---
Duplicate comments:
In `@src/SHiPGeometry.cpp`:
- Around line 58-62: Update src/SHiPGeometry.cpp lines 58-62 to parse the SND
envelope once, then pass its centre, size, and rotation to reserveSpace(...)
before MuonShieldFactory::build(), reusing the same values for
embedDaughter(...). Retain the carving description in
subsystems/MuonShield/README.md lines 24-42 and the reserveSpace claim in
subsystems/NeutrinoDetector/README.md lines 51-59 only after the builder wiring
is implemented.
In `@subsystems/MuonShield/src/MuonShieldFactory.cpp`:
- Around line 110-131: Update the carve-intersection logic around the
reservation loop to compute world-axis-aligned bounding boxes from all eight
transformed corners for both the block and each reservation, including their
rotations. Use those AABBs for overlap testing, and reject rotated blocks whose
transformed AABB exceeds the Air envelope before applying subtraction; preserve
the existing transformed reservation subtraction via resInBlock.
---
Nitpick comments:
In `@subsystems/MuonShield/src/MuonShieldFactory.cpp`:
- Line 81: Extend validation in MuonShieldFactory’s block-processing flow to
compare every pair of sibling block Z-intervals before placement, using
transformed bounds when rotation is enabled. Reject configurations with any
overlap while preserving the existing envelope validation, and add a test
covering overlapping block ranges.
In `@subsystems/MuonShield/test_muonshield.cpp`:
- Around line 89-93: Extend the test assertions after the existing magnet 1
check to verify that ms->getChildVol(5) has a GeoShapeSubtraction shape, while
preserving the current assertion for ms->getChildVol(6). This ensures both
reserved magnets are confirmed as carved.
🪄 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 Plus
Run ID: fb64bba5-f18d-47a4-9d56-1c090f759f94
📒 Files selected for processing (20)
CMakeLists.txtcmake/ShipAddTomlConfig.cmakeinclude/SHiPGeometry/TomlConfig.hsrc/SHiPGeometry.cppsubsystems/Calorimeter/CMakeLists.txtsubsystems/Calorimeter/src/CalorimeterConfig.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
| muon_shield (Air envelope, 3520 × 2640 × 27540 mm) centre z = 18.31 m | ||
| ├─ block_0 magnet 1 (Iron box) | ||
| ├─ … magnets 2–5 (Iron boxes; magnet 6 is a GeoTrd, widens in Y) | ||
| ├─ block_5 magnet 6 (Iron − SND box) ┐ carved by A − B | ||
| ├─ block_6 magnet 7 (Iron − SND box) ┘ (magnets the reservation intersects) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the stale magnet-6 shape description. Magnet 6 is asserted as a GeoBox, but both comments still describe it as a widening GeoTrd.
subsystems/MuonShield/README.md#L29-L33: describe magnet 6 as an iron box.subsystems/MuonShield/test_muonshield.cpp#L74-L75: update the stale test comment to match theGeoBoxassertion.
📍 Affects 2 files
subsystems/MuonShield/README.md#L29-L33(this comment)subsystems/MuonShield/test_muonshield.cpp#L74-L75
🤖 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/README.md` around lines 29 - 33, Update the magnet-6
description in subsystems/MuonShield/README.md lines 29-33 to identify it as an
iron box rather than a widening GeoTrd, and update the corresponding test
comment in subsystems/MuonShield/test_muonshield.cpp lines 74-75 to match the
GeoBox assertion; no code behavior changes are needed.
| for (std::size_t i = 0; i < cfg.blocks.size(); ++i) { | ||
| const auto& b = cfg.blocks[i]; | ||
| const std::string where = "block " + std::to_string(i) + " in " + path; | ||
| if (b.size[0] <= 0.0 || b.size[1] <= 0.0 || b.size[2] <= 0.0) | ||
| throw std::runtime_error("MuonShieldConfig: " + where + " has a non-positive size"); | ||
|
|
||
| // Taper must not collapse (or invert) the downstream face. | ||
| const double farHalfX = 0.5 * b.size[0] + b.size[2] * std::tan(b.taper_deg[0] * kDegToRad); | ||
| const double farHalfY = 0.5 * b.size[1] + b.size[2] * std::tan(b.taper_deg[1] * kDegToRad); | ||
| if (farHalfX <= 0.0 || farHalfY <= 0.0) | ||
| throw std::runtime_error("MuonShieldConfig: " + where + | ||
| " has a taper that collapses its downstream face"); | ||
|
|
||
| // The block must sit within the envelope in Z (light sanity check; | ||
| // rotations are not accounted for here). | ||
| if (b.start[2] < envStartMm - kEps || b.start[2] > envEndMm + kEps) | ||
| throw std::runtime_error("MuonShieldConfig: " + where + " has its upstream face (z = " + | ||
| std::to_string(b.start[2]) + " mm) outside the envelope"); | ||
| if (b.start[2] + b.size[2] > envEndMm + kEps) | ||
| throw std::runtime_error( | ||
| "MuonShieldConfig: " + where + " has its downstream face (z = " + | ||
| std::to_string(b.start[2] + b.size[2]) + " mm) beyond the envelope end"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Transverse (x/y) envelope check for blocks is missing — regression of a previously agreed fix.
A past review thread on this file requested this exact check ("Can we also check the envelope in x/y? For collisions with TCC8 this might be relevant.") and the author replied it had been added ("Each block's transverse extent (|start| + half-size, taper-aware) is now checked against envelope_half_x/y_mm..."). That check is absent from the current validation loop — only size positivity, taper-collapse, and Z-extent are validated. Cross-checked MuonShieldFactory.cpp's block-placement code too; no equivalent check exists there either. With MS.toml's magnet 7 already within ~50 mm of the x/y envelope limits, future edits could silently produce a magnet that pokes outside the Air container without any error, risking undetected TCC8 collisions.
🛡️ Proposed fix
// Taper must not collapse (or invert) the downstream face.
const double farHalfX = 0.5 * b.size[0] + b.size[2] * std::tan(b.taper_deg[0] * kDegToRad);
const double farHalfY = 0.5 * b.size[1] + b.size[2] * std::tan(b.taper_deg[1] * kDegToRad);
if (farHalfX <= 0.0 || farHalfY <= 0.0)
throw std::runtime_error("MuonShieldConfig: " + where +
" has a taper that collapses its downstream face");
+
+ // Transverse extent must stay within the envelope (taper-aware).
+ const double maxHalfX = std::max(0.5 * b.size[0], farHalfX);
+ const double maxHalfY = std::max(0.5 * b.size[1], farHalfY);
+ if (std::abs(b.start[0]) + maxHalfX > cfg.envelope_half_x_mm + kEps ||
+ std::abs(b.start[1]) + maxHalfY > cfg.envelope_half_y_mm + kEps)
+ throw std::runtime_error("MuonShieldConfig: " + where +
+ " extends beyond the envelope in x/y");🤖 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 139 - 161, Add
the missing transverse envelope validation inside the block loop alongside the
existing Z checks. In the validation flow using b.start, b.size, and taper-aware
downstream dimensions, compute each block’s maximum absolute x/y extent across
its tapered span and reject blocks exceeding envelope_half_x_mm or
envelope_half_y_mm, including the block location and offending extent in the
runtime error.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Distinct blocks are not checked for Z overlap.
This matches the open nitpick from the latest PR review ("validate that distinct blocks do not overlap in Z"). Blocks are listed sequentially in MS.toml but nothing enforces non-overlap; a future edit that shifts a block's start/size could silently create overlapping iron solids (GeoModel overlap), since only per-block Z-in-envelope bounds are checked here, not block-to-block relationships.
🛡️ Proposed fix (append after the per-block loop)
+ // Distinct blocks must not overlap in Z (order-independent).
+ std::vector<std::size_t> order(cfg.blocks.size());
+ for (std::size_t i = 0; i < order.size(); ++i) order[i] = i;
+ std::sort(order.begin(), order.end(), [&](std::size_t a, std::size_t b) {
+ return cfg.blocks[a].start[2] < cfg.blocks[b].start[2];
+ });
+ for (std::size_t k = 1; k < order.size(); ++k) {
+ const auto& prev = cfg.blocks[order[k - 1]];
+ const auto& cur = cfg.blocks[order[k]];
+ if (prev.start[2] + prev.size[2] > cur.start[2] + kEps)
+ throw std::runtime_error("MuonShieldConfig: blocks " + std::to_string(order[k - 1]) +
+ " and " + std::to_string(order[k]) + " overlap in z in " + path);
+ }
+
return cfg;📝 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.
| for (std::size_t i = 0; i < cfg.blocks.size(); ++i) { | |
| const auto& b = cfg.blocks[i]; | |
| const std::string where = "block " + std::to_string(i) + " in " + path; | |
| if (b.size[0] <= 0.0 || b.size[1] <= 0.0 || b.size[2] <= 0.0) | |
| throw std::runtime_error("MuonShieldConfig: " + where + " has a non-positive size"); | |
| // Taper must not collapse (or invert) the downstream face. | |
| const double farHalfX = 0.5 * b.size[0] + b.size[2] * std::tan(b.taper_deg[0] * kDegToRad); | |
| const double farHalfY = 0.5 * b.size[1] + b.size[2] * std::tan(b.taper_deg[1] * kDegToRad); | |
| if (farHalfX <= 0.0 || farHalfY <= 0.0) | |
| throw std::runtime_error("MuonShieldConfig: " + where + | |
| " has a taper that collapses its downstream face"); | |
| // The block must sit within the envelope in Z (light sanity check; | |
| // rotations are not accounted for here). | |
| if (b.start[2] < envStartMm - kEps || b.start[2] > envEndMm + kEps) | |
| throw std::runtime_error("MuonShieldConfig: " + where + " has its upstream face (z = " + | |
| std::to_string(b.start[2]) + " mm) outside the envelope"); | |
| if (b.start[2] + b.size[2] > envEndMm + kEps) | |
| throw std::runtime_error( | |
| "MuonShieldConfig: " + where + " has its downstream face (z = " + | |
| std::to_string(b.start[2] + b.size[2]) + " mm) beyond the envelope end"); | |
| } | |
| for (std::size_t i = 0; i < cfg.blocks.size(); ++i) { | |
| const auto& b = cfg.blocks[i]; | |
| const std::string where = "block " + std::to_string(i) + " in " + path; | |
| if (b.size[0] <= 0.0 || b.size[1] <= 0.0 || b.size[2] <= 0.0) | |
| throw std::runtime_error("MuonShieldConfig: " + where + " has a non-positive size"); | |
| // Taper must not collapse (or invert) the downstream face. | |
| const double farHalfX = 0.5 * b.size[0] + b.size[2] * std::tan(b.taper_deg[0] * kDegToRad); | |
| const double farHalfY = 0.5 * b.size[1] + b.size[2] * std::tan(b.taper_deg[1] * kDegToRad); | |
| if (farHalfX <= 0.0 || farHalfY <= 0.0) | |
| throw std::runtime_error("MuonShieldConfig: " + where + | |
| " has a taper that collapses its downstream face"); | |
| // The block must sit within the envelope in Z (light sanity check; | |
| // rotations are not accounted for here). | |
| if (b.start[2] < envStartMm - kEps || b.start[2] > envEndMm + kEps) | |
| throw std::runtime_error("MuonShieldConfig: " + where + " has its upstream face (z = " + | |
| std::to_string(b.start[2]) + " mm) outside the envelope"); | |
| if (b.start[2] + b.size[2] > envEndMm + kEps) | |
| throw std::runtime_error( | |
| "MuonShieldConfig: " + where + " has its downstream face (z = " + | |
| std::to_string(b.start[2] + b.size[2]) + " mm) beyond the envelope end"); | |
| } | |
| // Distinct blocks must not overlap in Z (order-independent). | |
| std::vector<std::size_t> order(cfg.blocks.size()); | |
| for (std::size_t i = 0; i < order.size(); ++i) order[i] = i; | |
| std::sort(order.begin(), order.end(), [&](std::size_t a, std::size_t b) { | |
| return cfg.blocks[a].start[2] < cfg.blocks[b].start[2]; | |
| }); | |
| for (std::size_t k = 1; k < order.size(); ++k) { | |
| const auto& prev = cfg.blocks[order[k - 1]]; | |
| const auto& cur = cfg.blocks[order[k]]; | |
| if (prev.start[2] + prev.size[2] > cur.start[2] + kEps) | |
| throw std::runtime_error("MuonShieldConfig: blocks " + std::to_string(order[k - 1]) + | |
| " and " + std::to_string(order[k]) + " overlap in z in " + path); | |
| } | |
| return cfg; |
🤖 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 139 - 161,
Validate distinct block Z intervals after the per-block checks in the
configuration validation flow, using each block’s start[2] and size[2]. Detect
any overlapping intervals, including unsorted blocks, and throw a runtime_error
identifying both conflicting block indices and the configuration path; retain
the existing envelope and taper validation unchanged.
| void MuonShieldFactory::embedDaughter(GeoPhysVol* daughter, double worldCentreZ_mm, | ||
| const std::string& name) { | ||
| m_daughters.push_back({daughter, worldCentreZ_mm, name}); | ||
| } | ||
|
|
||
| return stationPhys; | ||
| void MuonShieldFactory::reserveSpace(const std::array<double, 3>& worldCentre_mm, | ||
| const std::array<double, 3>& size_mm, | ||
| const std::array<double, 3>& rotation_deg) { | ||
| m_reservations.push_back({worldCentre_mm, size_mm, rotation_deg}); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject registrations after build and repeated builds.
These methods still accept mutations after the returned geometry is fixed, so those registrations are silently ignored; a second build() also recreates the hierarchy. Restore an m_built guard that throws before post-build mutations and repeat builds.
Also applies to: 61-62
🤖 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 44 - 52,
Restore the m_built guard in MuonShieldFactory::embedDaughter and reserveSpace
so they throw before accepting registrations after geometry has been built. Add
the same precondition to build() to reject repeated builds, preserving the
existing hierarchy creation only for the first build.
| * @param size_mm Full box dimensions {x, y, z} (mm). | ||
| * @param rotation_deg Optional extrinsic X->Y->Z rotation about the centre. | ||
| */ | ||
| void reserveSpace(const std::array<double, 3>& worldCentre_mm, |
There was a problem hiding this comment.
It seems like reserveSpace is now only used in the tests. Is this intentional? Should it not be called before embedDaughter?
| return resolveConfigPath(path, SD_TOML_DEFAULT_PATH, SD_TOML_INSTALL_PATH); | ||
| } | ||
|
|
||
| SNDEnvelope readSNDEnvelope(const std::string& rawPath) { |
There was a problem hiding this comment.
This is never actually called. Is this intentional?
| # The FairShip absorber (row 0) is the magnetised hadron stopper and lives in | ||
| # the Target subsystem (2.14-4.44 m); the shield front is at 2.14 m, so magnet 1 | ||
| # begins at 4.59 m. |
There was a problem hiding this comment.
This is not true yet. We'd need to add the hadron absorber there.
| 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"); |
There was a problem hiding this comment.
Can we take this number from the config instead of hardcoding it?
It's also z-only. The reservation (if it's called, see other comment) supports 3d-positioning and rotation, which could cause a mismatch.
There was a problem hiding this comment.
Can we please not prefix files and classes with Ship?
| 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) * |
There was a problem hiding this comment.
This is the same as in lines 125ff. Is there no 3D rotation in GeoModel? If not, maybe we can have a common helper in this file?
There was a problem hiding this comment.
Can we move the known keys check and readNumeric here as well, to remove further duplication?
|
|
||
| // Absolute fallback path baked in by CMake so out-of-source builds always find | ||
| // MS.toml even when the CWD doesn't contain a copy of it. | ||
| #ifndef MS_TOML_DEFAULT_PATH |
There was a problem hiding this comment.
I think now with the ship_add_toml_config CMake helper, this is no longer necessary, as it's always defined.
| #ifndef SD_TOML_DEFAULT_PATH | ||
| #define SD_TOML_DEFAULT_PATH "SD.toml" | ||
| #endif | ||
| #ifndef SD_TOML_INSTALL_PATH | ||
| #define SD_TOML_INSTALL_PATH "" | ||
| #endif | ||
|
|
There was a problem hiding this comment.
I think now with the ship_add_toml_config CMake helper, this is no longer necessary, as it's always defined.
| envelope_z_start_m = 4.5400 | ||
| envelope_z_end_m = 32.0800 |
There was a problem hiding this comment.
Since all other values are in mm, maybe we can just make these mm as well to avoid mistakes? This would also make a few unit conversions in MuonShieldConfig unnecessary.
| std::array<double, 3> centre_mm = {0.0, 0.0, 28950.0}; ///< world centre (mm) | ||
| std::array<double, 3> size_mm = {800.0, 800.0, 5100.0}; ///< full x, y, z (mm) |
There was a problem hiding this comment.
These seem unnecessary, as they are required in the SD.toml config.
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
MS.tomliron block list, with support for per-block rotation and optional taper.SD.tomlto reserve and carve out a clearance cavity to avoid overlaps.MS.toml/SD.toml.