Added SBT and SST in addition to calo#13
Open
matclim wants to merge 7 commits intoShipSoft:mainfrom
Open
Conversation
📝 WalkthroughWalkthroughThis PR integrates a new SBT (Surround Background Tagger) subsystem, refactors the calorimeter to use configuration-driven geometry with new helper classes for bar and fibre layers, implements detailed straw geometry for trackers with stereo views and shared volumes, and adds new materials (Lead, PVT, Polystyrene, Mylar, ArCO₂, LAB) to support detector construction. ChangesSBT Subsystem Integration
Calorimeter Config-Driven Refactoring
Trackers Detailed Straw Geometry
Sequence Diagram(s)sequenceDiagram
participant Calorimeter Build
participant CalorimeterFactory
participant CalorimeterConfig
participant CaloBarLayer
participant CaloFibreHPLayer
Calorimeter Build->>CalorimeterFactory: build()
CalorimeterFactory->>CalorimeterConfig: readCaloConfig(path)
CalorimeterConfig-->>CalorimeterFactory: CalorimeterConfig{layers, layers2, ...}
CalorimeterFactory->>CalorimeterFactory: Create container
CalorimeterFactory->>CalorimeterFactory: Compute module pitch & grid
loop for each module (mx, my)
CalorimeterFactory->>CalorimeterFactory: buildStack(cfg, mx, my, offset_x, offset_y)
CalorimeterFactory->>CaloBarLayer: place(layer_code=scintillator)
CaloBarLayer-->>CalorimeterFactory: bars placed (X or Y axis)
CalorimeterFactory->>CaloFibreHPLayer: build(layer_code=fiber)
CaloFibreHPLayer-->>CalorimeterFactory: fiber HPL placed (3 sublayers)
CalorimeterFactory-->>CalorimeterFactory: Next layer code
end
CalorimeterFactory-->>Calorimeter Build: GeoPhysVol* container
sequenceDiagram
participant Trackers Build
participant TrackersFactory
participant buildSharedLogVols
participant buildStation
participant placeLayer
participant placeSubLayer
Trackers Build->>TrackersFactory: build()
TrackersFactory->>buildSharedLogVols: buildSharedLogVols()
buildSharedLogVols->>buildSharedLogVols: Create shared GeoLogVol<br/>(straw wall, gas, envelopes, frame)
buildSharedLogVols-->>TrackersFactory: Cache GeoLogVol pointers
loop for each station (1..4)
TrackersFactory->>buildStation: buildStation(index)
buildStation->>buildStation: Create station GeoPhysVol
loop for each layer (1..4)
buildStation->>placeLayer: placeLayer(layer, stereo_angle)
placeLayer->>placeLayer: Create layer with frame
loop for each sublayer (nominal, shifted)
placeLayer->>placeSubLayer: placeSubLayer(shifted)
placeSubLayer->>placeSubLayer: For each straw along Y
placeSubLayer->>placeSubLayer: wall PhysVol + gas child<br/>(from shared logs)
placeSubLayer-->>placeLayer: Sublayer placed
end
placeLayer-->>buildStation: Layer with stereo rotation placed
end
buildStation-->>TrackersFactory: Station populated
end
TrackersFactory-->>Trackers Build: GeoPhysVol* container
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Author
|
There is an issue in the CMakes, I'll try to have a look tomorrow |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (4)
subsystems/Calorimeter/calo.cfg (1)
17-18: ⚡ Quick winStop reusing layer code
7for different materials.Here
7means lead inlayersbut iron inlayers2. That makes the config section-dependent and easy to decode incorrectly in future edits or parser refactors. Distinct codes or symbolic layer tokens would be safer.Also applies to: 31-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@subsystems/Calorimeter/calo.cfg` around lines 17 - 18, The config reuses numeric code 7 for different materials across sections (e.g., layers and layers2), making it ambiguous; update the config so each material has a unique, self-describing token (either distinct numbers or symbolic names) and replace all occurrences consistently (look for the keys "layers" and "layers2" and the later occurrences noted around lines 31-33) so that "lead" and "iron" do not share the same code; ensure any parser or consumer that expects numeric codes is updated to accept the new tokens or add a mapping table in the config that documents each code -> material mapping.tests/test_consistency.cpp (1)
155-161: ⚡ Quick winAdd an explicit sanity check for
/SHiP/sbt.This file now accounts for SBT in the count and overlap rules, but
PositionsSanitystill won't fail if/SHiP/sbtis present at the wrong Z. Adding its expected centre here would close that gap on the new subsystem.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_consistency.cpp` around lines 155 - 161, Add an explicit Expected entry for the SBT subsystem to the expected vector in tests/test_consistency.cpp so PositionsSanity will verify its Z; locate the std::vector<Expected> expected = { ... } and add a new entry with subsystem name "/SHiP/sbt", its correct centre Z (determine from the geometry/production files), and the same 500.0 tolerance (i.e., {"/SHiP/sbt", <correct_center_Z>, 500.0}) so the test fails if SBT is at the wrong Z.subsystems/Calorimeter/CMakeLists.txt (1)
41-41: 💤 Low valueSource-tree absolute path baked into installed binary.
CALO_CFG_DEFAULT_PATH="${CMAKE_CURRENT_SOURCE_DIR}/calo.cfg"embeds the machine-specific build-time path into the compiled library, breaking reproducible builds and portability. Consider guarding this definition with aCMAKE_BUILD_TYPEcheck (e.g.Debug/development only), or relying solely on the install-time path for deployed builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@subsystems/Calorimeter/CMakeLists.txt` at line 41, The build embeds an absolute source-tree path via CALO_CFG_DEFAULT_PATH="${CMAKE_CURRENT_SOURCE_DIR}/calo.cfg", which makes installed binaries non-portable; modify the CMake logic so CALO_CFG_DEFAULT_PATH is set to the source path only for development builds (guard with a CMAKE_BUILD_TYPE check against "Debug" or a custom DEV flag) and for install/release builds default to an install-time-relative path (e.g., the installed config location or a runtime-derivable path), ensuring the CALO_CFG_DEFAULT_PATH symbol is defined differently depending on build type rather than always using CMAKE_CURRENT_SOURCE_DIR.subsystems/Calorimeter/include/Calorimeter/CaloBarLayer.h (1)
25-27: 💤 Low valueLGTM. Minor:
tagPrefixtype inconsistency withnameSuffix.
nameSuffixisconst std::string&whiletagPrefixisconst char*. Harmonising both tostd::string_viewwould be cleaner, but this is purely cosmetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@subsystems/Calorimeter/include/Calorimeter/CaloBarLayer.h` around lines 25 - 27, The place(...) declaration in CaloBarLayer currently mixes const char* tagPrefix with const std::string& nameSuffix; change both to std::string_view (e.g. replace tagPrefix's type and nameSuffix's type) for consistency, add `#include` <string_view> in the header, update any default argument to use std::string_view{} or "" as appropriate, and update the corresponding implementation/definition of Calorimeter::CaloBarLayer::place to match the new std::string_view parameter types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Line 61: The build is failing because the SBT subsystem source tree is
missing; add a new subsystems/SBT/ directory containing at minimum a
CMakeLists.txt, the SBTFactory.h and SBTFactory.cpp (or equivalent) files, and
the GeoPhysVol*::build() implementation referenced by the factory so that the
add_subdirectory("SBT") call in subsystems/CMakeLists.txt and the install/target
reference in CMakeLists.txt can find the target; ensure the subsystem CMakeLists
defines a library/target named SBT and installs it (matching the name used by
src/CMakeLists.txt and any install(TARGETS SBT) calls) so the include of
SBTFactory.h and linking succeed.
In `@subsystems/Calorimeter/CMakeLists.txt`:
- Around line 37-50: The root CMakeLists currently calls
add_subdirectory(subsystems) before include(GNUInstallDirs), so
CMAKE_INSTALL_DATADIR is undefined when the Calorimeter target is configured;
move the include(GNUInstallDirs) line to earlier in the root CMakeLists so it
appears before the first add_subdirectory(subsystems) call, then reconfigure to
ensure CMAKE_INSTALL_DATADIR is set and that the Calorimeter install(DESTINATION
${CMAKE_INSTALL_DATADIR}/SHiPGeometry) and the CALO_CFG_INSTALL_PATH compile
definition receive the correct share path.
In `@subsystems/Calorimeter/include/Calorimeter/CalorimeterConfig.h`:
- Line 41: The default for airgap_mm in CalorimeterConfig (symbol airgap_mm in
CalorimeterConfig.h) is set to 1000.0 which is unrealistically large; change the
default to a realistic O(1–10 mm) value (e.g., 1.0 or 5.0) so omitted calo.cfg
entries won't introduce a 1 m gap, and run unit/stack-length tests that use
CalorimeterConfig to ensure computed stack lengths remain correct after the
change.
- Around line 62-64: The three config fields detector_offset_x_mm,
detector_offset_y_mm, detector_offset_z_mm declared in CalorimeterConfig.h (and
parsed in CalorimeterConfig.cpp) are dead because CalorimeterFactory::build()
ignores them and placement is hardcoded in SHiPGeometry.cpp (98.32 * 1000.0).
Either remove the three detector_offset_* fields and their parsing from
CalorimeterConfig.cpp (and any related docs) if offsets are not needed, or
update CalorimeterFactory::build() to read
CalorimeterConfig::detector_offset_x_mm / detector_offset_y_mm /
detector_offset_z_mm and use those values when computing the calorimeter
placement instead of the hardcoded 98.32*1000.0; also remove or replace the
hardcoded placement in SHiPGeometry.cpp accordingly and run tests/build to
ensure no references remain.
In `@subsystems/Calorimeter/src/CaloFibreHPLayer.cpp`:
- Around line 61-69: The current placement always uses three staggered sublayers
(dx, zLocal) and computes dz from casingZ and rOuter, which can lead to
overlapping rows or rows outside the casing when casingZ_mm <= fiberDiam_mm or
casingXY_mm is too small; update the logic around pitch, dxMax, usable, nFib,
x0, dx, dz and zLocal to first validate the casing dimensions and then adapt the
number/offset of sublayers instead of always using three: compute a usableZ =
casingZ - 2*rOuter and if usableZ < fiberDiam (or dz<=0) either reject/throw a
clear error or reduce rows to 1 (set zLocal to {0.0} and dx to {0.0}); likewise
compute usableXY and ensure the shifted middle sublayer (0.5*pitch) fits within
usable, reducing to 2 or 1 sublayers and recomputing nFib/x0 when it does not;
ensure you surface an explanatory error message when rejecting and update all
references that assume three rows to handle variable row counts.
In `@subsystems/Calorimeter/src/CalorimeterConfig.cpp`:
- Around line 122-125: The center_stack branch lowercases the raw token but
doesn't strip an optional trailing semicolon, so values like "true;" are treated
as false; before lowercasing and comparing in the else if (key ==
"center_stack") block, sanitize val by trimming surrounding whitespace and
removing a trailing ';' (if present) and then perform the lowercase + comparison
to set cfg.center_stack accordingly; update the logic in CalorimeterConfig.cpp
where cfg.center_stack is assigned to ensure the semicolon is stripped first.
- Around line 130-139: Add fast validation in CalorimeterConfig after parsing:
reject any non-positive geometry values by checking cfg.module_nx, cfg.module_ny
(if present), cfg.module_nz (if present), cfg.module_thickness_mm (or similar
module thickness field), cfg.fiber_diameter_mm, cfg.fiber_core_diameter_mm and
each layer's thickness (iterate cfg.layers and check layer.thickness_mm > 0),
and throw std::runtime_error with a clear message including the path if any are
<= 0; keep the existing fiber_core_diameter_mm > fiber_diameter_mm check and
ensure error texts reference CalorimeterConfig so callers in
CalorimeterFactory.cpp (which constructs GeoBox/GeoTube and module loops) fail
fast on invalid geometry.
In `@subsystems/Calorimeter/src/CalorimeterFactory.cpp`:
- Around line 65-87: totalStackZ in CalorimeterFactory (and the other
layer-handling blocks mentioned) incorrectly treats codes 1 and 2 as using
scint_thickness_mm; update the logic so that when iterating cfg.layers and
cfg.layers2 code==1 or code==2 you add cfg.pvt_thickness_mm (not
cfg.scint_thickness_mm) to the total and to any z advances; likewise, in the
geometry-building code paths that compute zCursor and pvtZ (and that size air
envelopes) ensure code 1/2 branches advance zCursor by pvtZ and use 0.5 * pvtZ
for air-envelope sizing instead of 0.5 * scint_thickness_mm; make analogous
fixes in the other functions/blocks cited (the branches around lines 155-179,
217-236, 317-336) so all uses of PVT layers consistently reference
pvt_thickness_mm.
- Around line 20-23: The file CalorimeterFactory.cpp uses std::move(configPath)
(see the call to std::move in the factory/create function) but does not directly
include <utility>; add a direct include for <utility> to the includes section so
std::move is declared (update the includes near the top where <fstream>,
<stdexcept>, <string> are listed).
In `@subsystems/CMakeLists.txt`:
- Line 10: The CMakeLists currently calls add_subdirectory(SBT) and later
installs target SBT but the SBT directory/target doesn't exist or has different
casing; add or rename the actual directory/target to match, or change the call
to the correct path/name. Specifically ensure a subsystems/SBT directory with a
CMakeLists that creates a target named SBT (or update add_subdirectory(...) and
install(TARGETS ... SBT) to the real directory/target name/casing) so
add_subdirectory(SBT) succeeds and install(TARGETS ... SBT) references an
existing target.
In `@subsystems/Trackers/README.md`:
- Around line 18-31: The fenced code block showing the tracker hierarchy (the
block starting with ``` and containing the /SHiP/trackers tree) is missing a
language tag; update that opening fence to include a language (e.g., change ```
to ```text) so markdownlint stops flagging it and the block is treated as plain
text in README.md.
---
Nitpick comments:
In `@subsystems/Calorimeter/calo.cfg`:
- Around line 17-18: The config reuses numeric code 7 for different materials
across sections (e.g., layers and layers2), making it ambiguous; update the
config so each material has a unique, self-describing token (either distinct
numbers or symbolic names) and replace all occurrences consistently (look for
the keys "layers" and "layers2" and the later occurrences noted around lines
31-33) so that "lead" and "iron" do not share the same code; ensure any parser
or consumer that expects numeric codes is updated to accept the new tokens or
add a mapping table in the config that documents each code -> material mapping.
In `@subsystems/Calorimeter/CMakeLists.txt`:
- Line 41: The build embeds an absolute source-tree path via
CALO_CFG_DEFAULT_PATH="${CMAKE_CURRENT_SOURCE_DIR}/calo.cfg", which makes
installed binaries non-portable; modify the CMake logic so CALO_CFG_DEFAULT_PATH
is set to the source path only for development builds (guard with a
CMAKE_BUILD_TYPE check against "Debug" or a custom DEV flag) and for
install/release builds default to an install-time-relative path (e.g., the
installed config location or a runtime-derivable path), ensuring the
CALO_CFG_DEFAULT_PATH symbol is defined differently depending on build type
rather than always using CMAKE_CURRENT_SOURCE_DIR.
In `@subsystems/Calorimeter/include/Calorimeter/CaloBarLayer.h`:
- Around line 25-27: The place(...) declaration in CaloBarLayer currently mixes
const char* tagPrefix with const std::string& nameSuffix; change both to
std::string_view (e.g. replace tagPrefix's type and nameSuffix's type) for
consistency, add `#include` <string_view> in the header, update any default
argument to use std::string_view{} or "" as appropriate, and update the
corresponding implementation/definition of Calorimeter::CaloBarLayer::place to
match the new std::string_view parameter types.
In `@tests/test_consistency.cpp`:
- Around line 155-161: Add an explicit Expected entry for the SBT subsystem to
the expected vector in tests/test_consistency.cpp so PositionsSanity will verify
its Z; locate the std::vector<Expected> expected = { ... } and add a new entry
with subsystem name "/SHiP/sbt", its correct centre Z (determine from the
geometry/production files), and the same 500.0 tolerance (i.e., {"/SHiP/sbt",
<correct_center_Z>, 500.0}) so the test fails if SBT is at the wrong Z.
🪄 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: 855149c0-3122-440f-8fd1-04058460082e
📒 Files selected for processing (21)
CMakeLists.txtsrc/CMakeLists.txtsrc/SHiPGeometry.cppsrc/SHiPMaterials.cppsubsystems/CMakeLists.txtsubsystems/Calorimeter/CMakeLists.txtsubsystems/Calorimeter/calo.cfgsubsystems/Calorimeter/include/Calorimeter/CaloBarLayer.hsubsystems/Calorimeter/include/Calorimeter/CaloFibreHPLayer.hsubsystems/Calorimeter/include/Calorimeter/CalorimeterConfig.hsubsystems/Calorimeter/include/Calorimeter/CalorimeterFactory.hsubsystems/Calorimeter/src/CaloBarLayer.cppsubsystems/Calorimeter/src/CaloFibreHPLayer.cppsubsystems/Calorimeter/src/CalorimeterConfig.cppsubsystems/Calorimeter/src/CalorimeterFactory.cppsubsystems/Calorimeter/test_calorimeter.cppsubsystems/Trackers/README.mdsubsystems/Trackers/include/Trackers/TrackersFactory.hsubsystems/Trackers/src/TrackersFactory.cppsubsystems/Trackers/test_trackers.cpptests/test_consistency.cpp
| MuonShield | ||
| Magnet | ||
| DecayVolume | ||
| SBT |
There was a problem hiding this comment.
Missing subsystems/SBT/ directory blocks the entire build.
Both pipeline errors — add_subdirectory given source "SBT" which is not an existing directory (in subsystems/CMakeLists.txt) and install TARGETS given target "SBT" which does not exist (here) — cascade from the same root cause: the SBT subsystem source tree was not committed. The SBTFactory.h include and the link in src/CMakeLists.txt are also blocked.
The subsystems/SBT/ directory with at minimum a CMakeLists.txt, the SBTFactory header/source, and its GeoPhysVol* build() implementation must be added before this PR can merge.
Also applies to: 82-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CMakeLists.txt` at line 61, The build is failing because the SBT subsystem
source tree is missing; add a new subsystems/SBT/ directory containing at
minimum a CMakeLists.txt, the SBTFactory.h and SBTFactory.cpp (or equivalent)
files, and the GeoPhysVol*::build() implementation referenced by the factory so
that the add_subdirectory("SBT") call in subsystems/CMakeLists.txt and the
install/target reference in CMakeLists.txt can find the target; ensure the
subsystem CMakeLists defines a library/target named SBT and installs it
(matching the name used by src/CMakeLists.txt and any install(TARGETS SBT)
calls) so the include of SBTFactory.h and linking succeed.
Comment on lines
+37
to
+50
| target_compile_definitions( | ||
| Calorimeter | ||
| PRIVATE | ||
| # Source-tree fallback (always valid during development and CI builds). | ||
| CALO_CFG_DEFAULT_PATH="${CMAKE_CURRENT_SOURCE_DIR}/calo.cfg" | ||
| # Install-time fallback: set to the installed data directory so that | ||
| # deployed builds (where the source tree is absent) can find calo.cfg. | ||
| CALO_CFG_INSTALL_PATH="${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_DATADIR}/SHiPGeometry/calo.cfg" | ||
| ) | ||
|
|
||
| install( | ||
| FILES ${CMAKE_CURRENT_SOURCE_DIR}/calo.cfg | ||
| DESTINATION ${CMAKE_INSTALL_DATADIR}/SHiPGeometry | ||
| ) |
There was a problem hiding this comment.
${CMAKE_INSTALL_DATADIR} is empty when this file is processed.
In the root CMakeLists.txt, add_subdirectory(subsystems) (line 39) runs before include(GNUInstallDirs) (line 49). CMAKE_INSTALL_DATADIR is only defined by GNUInstallDirs, so at configure time it is an empty string here. Consequences:
install(DESTINATION ${CMAKE_INSTALL_DATADIR}/SHiPGeometry)resolves to<prefix>/SHiPGeometryinstead of<prefix>/share/SHiPGeometry.CALO_CFG_INSTALL_PATHgets a double-slash (<prefix>//SHiPGeometry/calo.cfg), which is harmless on POSIX but still incorrect.
The fix is to hoist include(GNUInstallDirs) in the root CMakeLists.txt to before the first add_subdirectory call:
🛠️ Proposed fix in `CMakeLists.txt` (root)
find_package(SQLite3 REQUIRED)
+include(GNUInstallDirs)
+include(CMakePackageConfigHelpers)
+
# Testing
include(CTest)
...
# ----------------------------------------------------------------------------
# Installation
# ----------------------------------------------------------------------------
-include(GNUInstallDirs)
-include(CMakePackageConfigHelpers)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@subsystems/Calorimeter/CMakeLists.txt` around lines 37 - 50, The root
CMakeLists currently calls add_subdirectory(subsystems) before
include(GNUInstallDirs), so CMAKE_INSTALL_DATADIR is undefined when the
Calorimeter target is configured; move the include(GNUInstallDirs) line to
earlier in the root CMakeLists so it appears before the first
add_subdirectory(subsystems) call, then reconfigure to ensure
CMAKE_INSTALL_DATADIR is set and that the Calorimeter install(DESTINATION
${CMAKE_INSTALL_DATADIR}/SHiPGeometry) and the CALO_CFG_INSTALL_PATH compile
definition receive the correct share path.
| double hpl_thickness_mm = 10.0; | ||
| double fiber_diameter_mm = 1.2; | ||
| double fiber_core_diameter_mm = -1.0; // <0 → use fiber_diameter_mm | ||
| double airgap_mm = 1000.0; |
There was a problem hiding this comment.
airgap_mm default of 1000 mm (1 m) looks wrong.
Layer code 8 is defined as "no volume placed, just advances z cursor". A 1-metre default for an in-sequence air gap is implausibly large for a calorimeter layer and could corrupt the computed stack length if calo.cfg ever omits this key. Verify the intended default (likely O(1–10 mm)).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@subsystems/Calorimeter/include/Calorimeter/CalorimeterConfig.h` at line 41,
The default for airgap_mm in CalorimeterConfig (symbol airgap_mm in
CalorimeterConfig.h) is set to 1000.0 which is unrealistically large; change the
default to a realistic O(1–10 mm) value (e.g., 1.0 or 5.0) so omitted calo.cfg
entries won't introduce a 1 m gap, and run unit/stack-length tests that use
CalorimeterConfig to ensure computed stack lengths remain correct after the
change.
Comment on lines
+62
to
+64
| double detector_offset_x_mm = 0.0; | ||
| double detector_offset_y_mm = 0.0; | ||
| double detector_offset_z_mm = 96970.0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find CalorimeterFactory::build() and check whether detector_offset_z_mm is read
rg -n "detector_offset" --type=cpp -A2 -B2Repository: ShipSoft/Geometry
Length of output: 1786
🏁 Script executed:
#!/bin/bash
# Search for usages of detector_offset_z_mm throughout the codebase
rg "detector_offset_z_mm" --type=cpp --type=h -nRepository: ShipSoft/Geometry
Length of output: 378
🏁 Script executed:
#!/bin/bash
# Find and examine CalorimeterFactory::build() implementation
rg -n "class CalorimeterFactory|::build\(" subsystems/Calorimeter --type=cpp --type=h -A5Repository: ShipSoft/Geometry
Length of output: 4609
🏁 Script executed:
#!/bin/bash
# Locate SHiPGeometry files to examine calorimeter placement
fd -i "shipgeometry" --type fRepository: ShipSoft/Geometry
Length of output: 152
🏁 Script executed:
#!/bin/bash
# Get full CalorimeterFactory::build() implementation
sed -n '99,500p' subsystems/Calorimeter/src/CalorimeterFactory.cpp | head -100Repository: ShipSoft/Geometry
Length of output: 5787
🏁 Script executed:
#!/bin/bash
# Check the end of build() to see return/transform statement
sed -n '99,550p' subsystems/Calorimeter/src/CalorimeterFactory.cpp | tail -50Repository: ShipSoft/Geometry
Length of output: 2099
🏁 Script executed:
#!/bin/bash
# Search for where detector_offset is accessed in the config
rg "cfg\." subsystems/Calorimeter/src/CalorimeterFactory.cpp | head -30Repository: ShipSoft/Geometry
Length of output: 1753
🏁 Script executed:
#!/bin/bash
# Examine SHiPGeometry.cpp for calorimeter container placement
wc -l src/SHiPGeometry.cppRepository: ShipSoft/Geometry
Length of output: 86
🏁 Script executed:
#!/bin/bash
# Read the entire SHiPGeometry.cpp file
cat -n src/SHiPGeometry.cppRepository: ShipSoft/Geometry
Length of output: 7313
🏁 Script executed:
#!/bin/bash
# Search for calorimeter positioning and any reference to 98320 or similar Z position
rg -n "98|calorimeter|Calorimeter" src/SHiPGeometry.cpp -B2 -A2Repository: ShipSoft/Geometry
Length of output: 964
detector_offset_z_mm appears to be unused dead code.
The field is parsed in CalorimeterConfig.cpp but never accessed within CalorimeterFactory::build(). The actual calorimeter placement is hardcoded in SHiPGeometry.cpp line 139 at 98.32 * 1000.0 = 98320 mm, making the default value 96970.0 in the header file ineffectual. Remove the unused detector_offset_* fields or integrate them into the factory if they are intended to drive positioning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@subsystems/Calorimeter/include/Calorimeter/CalorimeterConfig.h` around lines
62 - 64, The three config fields detector_offset_x_mm, detector_offset_y_mm,
detector_offset_z_mm declared in CalorimeterConfig.h (and parsed in
CalorimeterConfig.cpp) are dead because CalorimeterFactory::build() ignores them
and placement is hardcoded in SHiPGeometry.cpp (98.32 * 1000.0). Either remove
the three detector_offset_* fields and their parsing from CalorimeterConfig.cpp
(and any related docs) if offsets are not needed, or update
CalorimeterFactory::build() to read CalorimeterConfig::detector_offset_x_mm /
detector_offset_y_mm / detector_offset_z_mm and use those values when computing
the calorimeter placement instead of the hardcoded 98.32*1000.0; also remove or
replace the hardcoded placement in SHiPGeometry.cpp accordingly and run
tests/build to ensure no references remain.
Comment on lines
+61
to
+69
| const double pitch = 2.0 * rOuter; // tight packing | ||
| const double dxMax = 0.5 * pitch; // middle-sublayer shift | ||
| const double usable = casingXY - 2.0 * rOuter - dxMax; | ||
| const int nFib = std::max(1, static_cast<int>(std::floor(usable / pitch)) + 1); | ||
| const double x0 = -0.5 * ((nFib - 1) * pitch + dxMax); | ||
|
|
||
| const double dx[3] = {0.0, 0.5 * pitch, 0.0}; | ||
| const double dz = std::max(0.0, 0.5 * casingZ - rOuter); | ||
| const double zLocal[3] = {-dz, 0.0, +dz}; |
There was a problem hiding this comment.
Validate that the fixed 3-row packing actually fits the casing.
This code always generates three staggered sublayers. For undersized inputs, that produces invalid geometry: e.g. when casingZ_mm <= fiberDiam_mm, dz collapses to 0 and all three rows overlap at the same Z; with a narrow casingXY_mm, the shifted row can extend outside the box. Please reject too-small casings or reduce the packing pattern based on the available space before placement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@subsystems/Calorimeter/src/CaloFibreHPLayer.cpp` around lines 61 - 69, The
current placement always uses three staggered sublayers (dx, zLocal) and
computes dz from casingZ and rOuter, which can lead to overlapping rows or rows
outside the casing when casingZ_mm <= fiberDiam_mm or casingXY_mm is too small;
update the logic around pitch, dxMax, usable, nFib, x0, dx, dz and zLocal to
first validate the casing dimensions and then adapt the number/offset of
sublayers instead of always using three: compute a usableZ = casingZ - 2*rOuter
and if usableZ < fiberDiam (or dz<=0) either reject/throw a clear error or
reduce rows to 1 (set zLocal to {0.0} and dx to {0.0}); likewise compute
usableXY and ensure the shifted middle sublayer (0.5*pitch) fits within usable,
reducing to 2 or 1 sublayers and recomputing nFib/x0 when it does not; ensure
you surface an explanatory error message when rejecting and update all
references that assume three rows to handle variable row counts.
Comment on lines
+130
to
+139
| if (cfg.layers.empty()) | ||
| throw std::runtime_error("CalorimeterConfig: 'layers' must be defined in " + path); | ||
|
|
||
| // Default fibre-core diameter to outer diameter when not set | ||
| if (cfg.fiber_core_diameter_mm <= 0) | ||
| cfg.fiber_core_diameter_mm = cfg.fiber_diameter_mm; | ||
|
|
||
| if (cfg.fiber_core_diameter_mm > cfg.fiber_diameter_mm) | ||
| throw std::runtime_error( | ||
| "CalorimeterConfig: fiber_core_diameter_mm cannot exceed fiber_diameter_mm"); |
There was a problem hiding this comment.
Reject non-positive geometry values while loading the config.
Right now a typo like module_nx = 0 or a non-positive thickness survives parsing and only fails later as invalid geometry, or worse, builds an empty calorimeter. Since these values are consumed directly by CalorimeterFactory.cpp for GeoBox/GeoTube dimensions and module loops, this should fail fast here.
Suggested fix
if (cfg.layers.empty())
throw std::runtime_error("CalorimeterConfig: 'layers' must be defined in " + path);
+
+ auto requirePositive = [&](double value, const char* name) {
+ if (value <= 0.0)
+ throw std::runtime_error("CalorimeterConfig: '" + std::string(name) +
+ "' must be > 0 in " + path);
+ };
+
+ requirePositive(cfg.plate_xy_mm, "plate_xy_mm");
+ requirePositive(cfg.lead_thickness_mm, "lead_thickness_mm");
+ requirePositive(cfg.scint_thickness_mm, "scint_thickness_mm");
+ requirePositive(cfg.pvt_thickness_mm, "pvt_thickness_mm");
+ requirePositive(cfg.hpl_thickness_mm, "hpl_thickness_mm");
+ requirePositive(cfg.fiber_diameter_mm, "fiber_diameter_mm");
+ requirePositive(cfg.iron_thickness_mm, "iron_thickness_mm");
+
+ if (cfg.module_nx <= 0 || cfg.module_ny <= 0)
+ throw std::runtime_error("CalorimeterConfig: module_nx and module_ny must be > 0 in " +
+ path);
// Default fibre-core diameter to outer diameter when not set
if (cfg.fiber_core_diameter_mm <= 0)
cfg.fiber_core_diameter_mm = cfg.fiber_diameter_mm;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@subsystems/Calorimeter/src/CalorimeterConfig.cpp` around lines 130 - 139, Add
fast validation in CalorimeterConfig after parsing: reject any non-positive
geometry values by checking cfg.module_nx, cfg.module_ny (if present),
cfg.module_nz (if present), cfg.module_thickness_mm (or similar module thickness
field), cfg.fiber_diameter_mm, cfg.fiber_core_diameter_mm and each layer's
thickness (iterate cfg.layers and check layer.thickness_mm > 0), and throw
std::runtime_error with a clear message including the path if any are <= 0; keep
the existing fiber_core_diameter_mm > fiber_diameter_mm check and ensure error
texts reference CalorimeterConfig so callers in CalorimeterFactory.cpp (which
constructs GeoBox/GeoTube and module loops) fail fast on invalid geometry.
Comment on lines
+20
to
+23
| #include <fstream> | ||
| #include <stdexcept> | ||
| #include <string> | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== File header =="
fd 'CalorimeterFactory.cpp$' | while read -r f; do
sed -n '1,70p' "$f"
done
echo
echo "== std::move call sites =="
rg -n '\bstd::move\s*\(' --type=cpp
echo
echo "== Explicit <utility> includes =="
rg -n '^#include <utility>$' --type=cppRepository: ShipSoft/Geometry
Length of output: 2797
Add #include <utility> for std::move.
Line 57 uses std::move(configPath) but <utility> is not included. While this may compile via transitive includes, it is non-portable and breaks with stricter standard library implementations. Add #include <utility> to the includes section.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@subsystems/Calorimeter/src/CalorimeterFactory.cpp` around lines 20 - 23, The
file CalorimeterFactory.cpp uses std::move(configPath) (see the call to
std::move in the factory/create function) but does not directly include
<utility>; add a direct include for <utility> to the includes section so
std::move is declared (update the includes near the top where <fstream>,
<stdexcept>, <string> are listed).
Comment on lines
+65
to
+87
| double CalorimeterFactory::totalStackZ(const CalorimeterConfig& cfg) { | ||
| double total = 0.0; | ||
| for (int code : cfg.layers) { | ||
| if (code == 7) | ||
| total += cfg.lead_thickness_mm; | ||
| else if (code >= 1 && code <= 4) | ||
| total += cfg.scint_thickness_mm; | ||
| else if (code == 5 || code == 6) | ||
| total += cfg.hpl_thickness_mm; | ||
| else if (code == 8) | ||
| total += cfg.airgap_mm; | ||
| else | ||
| throw std::runtime_error("CalorimeterFactory: unknown layer code in 'layers': " + | ||
| std::to_string(code)); | ||
| } | ||
| total += cfg.gap_ecal_hcal_mm; | ||
| for (int code : cfg.layers2) { | ||
| if (code == 7) | ||
| total += cfg.iron_thickness_mm; | ||
| else if (code >= 1 && code <= 4) | ||
| total += cfg.scint_thickness_mm; | ||
| else if (code == 5 || code == 6) | ||
| total += cfg.hpl_thickness_mm; |
There was a problem hiding this comment.
Use pvt_thickness_mm for codes 1/2 instead of folding everything into scint_thickness_mm.
CalorimeterConfig carries separate pvt_thickness_mm and scint_thickness_mm, but this implementation never reads pvt_thickness_mm. As written, the wide PVT layers are built with scint_thickness_mm, and totalStackZ() makes the same assumption, so a config with different PVT and PS/scint thicknesses produces the wrong geometry and the wrong stack-depth check.
Suggested fix
double CalorimeterFactory::totalStackZ(const CalorimeterConfig& cfg) {
double total = 0.0;
for (int code : cfg.layers) {
if (code == 7)
total += cfg.lead_thickness_mm;
- else if (code >= 1 && code <= 4)
+ else if (code == 1 || code == 2)
+ total += cfg.pvt_thickness_mm;
+ else if (code == 3 || code == 4)
total += cfg.scint_thickness_mm;
else if (code == 5 || code == 6)
total += cfg.hpl_thickness_mm;
else if (code == 8)
total += cfg.airgap_mm;
@@
total += cfg.gap_ecal_hcal_mm;
for (int code : cfg.layers2) {
if (code == 7)
total += cfg.iron_thickness_mm;
- else if (code >= 1 && code <= 4)
+ else if (code == 1 || code == 2)
+ total += cfg.pvt_thickness_mm;
+ else if (code == 3 || code == 4)
total += cfg.scint_thickness_mm;
else if (code == 5 || code == 6)
total += cfg.hpl_thickness_mm;
else if (code == 8)
total += cfg.airgap_mm;
@@
- const double scintZ = cfg.scint_thickness_mm * mm;
+ const double pvtZ = cfg.pvt_thickness_mm * mm;
+ const double scintZ = cfg.scint_thickness_mm * mm;
@@
auto* wideHLog = new GeoLogVol("/SHiP/calorimeter/wide_pvt_h" + mtag,
- new GeoBox(0.5 * plateXY, 0.5 * wideW, 0.5 * scintZ), pvtMat);
+ new GeoBox(0.5 * plateXY, 0.5 * wideW, 0.5 * pvtZ), pvtMat);
auto* wideVLog = new GeoLogVol("/SHiP/calorimeter/wide_pvt_v" + mtag,
- new GeoBox(0.5 * wideW, 0.5 * plateXY, 0.5 * scintZ), pvtMat);
+ new GeoBox(0.5 * wideW, 0.5 * plateXY, 0.5 * pvtZ), pvtMat);You would also want the code-1 / code-2 branches to advance zCursor by pvtZ and to size their air envelopes with 0.5 * pvtZ.
Also applies to: 155-179, 217-236, 317-336
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@subsystems/Calorimeter/src/CalorimeterFactory.cpp` around lines 65 - 87,
totalStackZ in CalorimeterFactory (and the other layer-handling blocks
mentioned) incorrectly treats codes 1 and 2 as using scint_thickness_mm; update
the logic so that when iterating cfg.layers and cfg.layers2 code==1 or code==2
you add cfg.pvt_thickness_mm (not cfg.scint_thickness_mm) to the total and to
any z advances; likewise, in the geometry-building code paths that compute
zCursor and pvtZ (and that size air envelopes) ensure code 1/2 branches advance
zCursor by pvtZ and use 0.5 * pvtZ for air-envelope sizing instead of 0.5 *
scint_thickness_mm; make analogous fixes in the other functions/blocks cited
(the branches around lines 155-179, 217-236, 317-336) so all uses of PVT layers
consistently reference pvt_thickness_mm.
| add_subdirectory(MuonShield) | ||
| add_subdirectory(Magnet) | ||
| add_subdirectory(DecayVolume) | ||
| add_subdirectory(SBT) |
There was a problem hiding this comment.
Wire in SBT only after the directory/target exists.
This line currently breaks configure: CI reports add_subdirectory(SBT) cannot find the directory, and the later install(TARGETS ... SBT) fails for the same reason. Please add the missing subsystems/SBT CMake target in this PR or point this at the actual directory/casing.
🧰 Tools
🪛 GitHub Actions: Build and Test
[error] 10-10: CMake error: add_subdirectory given source "SBT" which is not an existing directory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@subsystems/CMakeLists.txt` at line 10, The CMakeLists currently calls
add_subdirectory(SBT) and later installs target SBT but the SBT directory/target
doesn't exist or has different casing; add or rename the actual directory/target
to match, or change the call to the correct path/name. Specifically ensure a
subsystems/SBT directory with a CMakeLists that creates a target named SBT (or
update add_subdirectory(...) and install(TARGETS ... SBT) to the real
directory/target name/casing) so add_subdirectory(SBT) succeeds and
install(TARGETS ... SBT) references an existing target.
Comment on lines
18
to
31
| ``` | ||
| TrackersContainer (Air, 6000×6860×6000 mm) | ||
| ├─ TrackerStation_1 (Air, 6000×6860×1000 mm) z = 84070 mm | ||
| ├─ TrackerStation_2 (Air) z = 86070 mm | ||
| ├─ TrackerStation_3 (Air) z = 93070 mm | ||
| └─ TrackerStation_4 (Air) z = 95070 mm | ||
| /SHiP/trackers (Air, 6000 × 6860 × 12000 mm) | ||
| ├─ /SHiP/trackers/station_1 (Air, 6000 × 6860 × 1000 mm) z = 84070 mm | ||
| │ └─ Layer_j [j = 0..3] air box, rotated by ±2.3° about Z | ||
| │ ├─ StrawFrame hollow rectangle (Aluminium, GeoShapeSubtraction) | ||
| │ │ outer 4210 × 6230 mm, aperture 4010 × 6030 mm | ||
| │ ├─ StrawSubLayer_0 air slab at z = -10.55 mm (nominal) | ||
| │ │ └─ Straw [i = 0..299] wall (Mylar) + gas (Ar/CO₂) tubes | ||
| │ └─ StrawSubLayer_1 air slab at z = +10.55 mm (Y-staggered) | ||
| │ └─ Straw [i = 0..299] | ||
| ├─ /SHiP/trackers/station_2 z = 86070 mm | ||
| ├─ /SHiP/trackers/station_3 z = 93070 mm | ||
| └─ /SHiP/trackers/station_4 z = 95070 mm | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to this fenced block.
The fence starting on Line 18 is missing a language, and markdownlint is already flagging it. text would be enough here.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 18-18: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@subsystems/Trackers/README.md` around lines 18 - 31, The fenced code block
showing the tracker hierarchy (the block starting with ``` and containing the
/SHiP/trackers tree) is missing a language tag; update that opening fence to
include a language (e.g., change ``` to ```text) so markdownlint stops flagging
it and the block is treated as plain text in README.md.
Contributor
|
@matclim Does this supersede 11 or should it be merged after? |
Author
|
Either way should work, it does technically superseed it however as 11 is included in this one |
Contributor
I'd prefer we first merge the calo and then rebase this. As is, it's very difficult to review for me. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Documentation