updated with calorimeter, to be checked#11
Open
matclim wants to merge 7 commits intoShipSoft:mainfrom
Open
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:
📝 WalkthroughWalkthroughCalorimeter construction was converted from fixed ECAL/HCAL sections to a TOML-driven, layer-by-layer builder. CalorimeterFactory accepts/resolves a config path, loads CalorimeterConfig, validates geometry, and places tiled per-module stacks. New config parsing, bar/fibre layer builders, tests, CMake wiring, and three materials (Lead, PVT, Polystyrene) added. ChangesCalorimeter (config-driven layout)
Materials & Minor edits
Sequence DiagramsequenceDiagram
participant Client as rgba(52,152,219,0.5) Client
participant Factory as rgba(46,204,113,0.5) CalorimeterFactory
participant CfgResolver as rgba(241,196,15,0.5) ConfigResolver
participant CfgLoader as rgba(155,89,182,0.5) ConfigLoader
participant Builder as rgba(230,126,34,0.5) buildStack
participant BarLayer as rgba(52,73,94,0.5) CaloBarLayer
participant FiberLayer as rgba(192,57,43,0.5) CaloFibreHPLayer
participant Container as rgba(127,140,141,0.5) ContainerVolume
Client->>Factory: build()
Factory->>CfgResolver: resolveCfgPath(m_configPath)
CfgResolver-->>Factory: resolved path
Factory->>CfgLoader: readCaloConfig(path)
CfgLoader-->>Factory: CalorimeterConfig
Factory->>Factory: totalStackZ(cfg) / validate fit
loop per module (mx,my)
Factory->>Builder: buildStack(container,cfg,mx,my,offs)
loop per layer in cfg.layers/cfg.layers2
alt bar layer
Builder->>BarLayer: place(...)
BarLayer-->>Builder: placed bars
else fibre layer
Builder->>FiberLayer: build(...)
FiberLayer-->>Builder: built HPL
else air / gap
Builder->>Builder: advance Z cursor
end
Builder->>Container: place layer envelope
end
end
Factory-->>Client: GeoPhysVol* (container)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 4
🧹 Nitpick comments (1)
subsystems/Calorimeter/test_calorimeter.cpp (1)
32-39: Make the child-count assertion stronger.
getNChildVols() >= 1only proves the container is non-empty. This refactor can still drop most layers/modules and keep the test green. Please assert the expected count from the stagedcalo.cfg, or at least a lower bound derived frommodule_nx * module_ny * populated_layer_count.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@subsystems/Calorimeter/test_calorimeter.cpp` around lines 32 - 39, Update the test CalorimeterHasChildren to assert a stronger expected child count: instead of CHECK(calo->getNChildVols() >= 1u), compute or hardcode the expected lower bound from the staged configuration and assert equality or a tighter >=, e.g. read module_nx, module_ny and populated_layer_count (or the expected total layers from calo.cfg) and assert calo->getNChildVols() >= module_nx * module_ny * populated_layer_count (or == expected_total_children); modify the test to access SHiPMaterials/CalorimeterFactory configuration values (or parse calo.cfg) and replace the loose check on calo->getNChildVols() with the computed stronger assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/SHiPMaterials.cpp`:
- Around line 194-208: The PVT and Polystyrene materials are being added with
raw atom counts (stoichiometry) instead of normalized mass fractions; update the
GeoMaterial creation for "PVT" and "Polystyrene" to compute mass fractions from
atomic masses (use m_elements["Carbon"] and m_elements["Hydrogen"] to get each
element's atomic weight), compute the molecule molecular weight (e.g., MW =
9*AW_C + 10*AW_H for PVT), then add normalized fractions (count*AW_element / MW)
to the GeoMaterial before lock(), and keep storing them in m_materials["PVT"]
and m_materials["Polystyrene"] as before.
In `@subsystems/Calorimeter/CMakeLists.txt`:
- Around line 34-43: The compile-time fallback CALO_CFG_DEFAULT_PATH currently
embeds the source-tree path but the installed calo.cfg is not probed; update the
build and runtime lookup so installed data is found: add a second compile
definition (or an exported header) that contains the install data directory path
(the ${CMAKE_INSTALL_DATADIR}/SHiPGeometry location) when installing the
Calorimeter target, and modify resolveCfgPath() in CalorimeterFactory.cpp to
check that install-time data-dir path in addition to the working directory and
CALO_CFG_DEFAULT_PATH (use the unique symbol CALO_CFG_DEFAULT_PATH and the
resolveCfgPath function name to locate the places to change). Ensure the
install-time path is only set when performing an install so local development
still falls back to the source-tree path.
In `@subsystems/Calorimeter/src/CalorimeterFactory.cpp`:
- Around line 261-315: The loop in CalorimeterFactory.cpp that iterates
cfg.layers2 accepts codes 1,2,5,6,7,8 but not 3 or 4, causing a runtime throw
while totalStackZ() treats 1..4 as valid; fix by either (A) adding explicit
branches for codes 3 and 4 that create the expected thin-PS scintillator layers
(mirror the pattern used for code 1/2: build an env via makeEnv and call the
appropriate placement/build routine, increment zCursor and indices like sl/gl),
or (B) validate/normalize cfg.layers2 earlier (e.g. in totalStackZ() caller or
CalorimeterFactory constructor) to reject or translate codes 3 and 4 and throw a
clear config error; reference the loop over cfg.layers2, totalStackZ(), makeEnv,
CaloBarLayer::place and CaloFibreHPLayer::build when making the change.
- Around line 83-107: Before placing modules, validate that the calorimeter
stack and tiled footprint fit inside the fixed container: compute total Z extent
using totalStackZ(cfg) and compute full X/Y extents from cfg.module_nx,
cfg.module_ny and effective pitch (use same logic as pitchX/pitchY:
cfg.module_pitch_x_mm>0 ? cfg.module_pitch_x_mm : cfg.plate_xy_mm), then compare
half-extents against s_containerHalfZ and s_containerHalfX/s_containerHalfY
respectively; if any extent exceeds the container, throw a clear runtime_error
(or appropriate config exception) with a descriptive message referencing the
offending dimension and the computed vs allowed size before the loop that calls
buildStack(containerPhys, cfg, ...).
---
Nitpick comments:
In `@subsystems/Calorimeter/test_calorimeter.cpp`:
- Around line 32-39: Update the test CalorimeterHasChildren to assert a stronger
expected child count: instead of CHECK(calo->getNChildVols() >= 1u), compute or
hardcode the expected lower bound from the staged configuration and assert
equality or a tighter >=, e.g. read module_nx, module_ny and
populated_layer_count (or the expected total layers from calo.cfg) and assert
calo->getNChildVols() >= module_nx * module_ny * populated_layer_count (or ==
expected_total_children); modify the test to access
SHiPMaterials/CalorimeterFactory configuration values (or parse calo.cfg) and
replace the loose check on calo->getNChildVols() with the computed stronger
assertion.
🪄 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: 33c8398b-10cc-494d-a272-1a3039478fd2
📒 Files selected for processing (11)
.clang-format.github/dependabot.yml.github/workflows/build-test.yml.gitignore.pre-commit-config.yamlsrc/SHiPGeometry.cppsrc/SHiPMaterials.cppsubsystems/Calorimeter/CMakeLists.txtsubsystems/Calorimeter/include/Calorimeter/CalorimeterFactory.hsubsystems/Calorimeter/src/CalorimeterFactory.cppsubsystems/Calorimeter/test_calorimeter.cpp
💤 Files with no reviewable changes (5)
- .github/dependabot.yml
- .clang-format
- .gitignore
- .github/workflows/build-test.yml
- .pre-commit-config.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@subsystems/Calorimeter/src/CalorimeterFactory.cpp`:
- Around line 185-197: layerId is reset to 0 inside buildStack(), so
GeoIdentifierTag instances created by makeEnv are duplicated across modules
(mx,my) when all layers are placed under the same container; change the code so
identifiers are globally unique per module by either (A) making a per-module
parent GeoPhysVol and attach layers to that parent instead of the shared
container, or (B) incorporate module coordinates into the identifier before
creating GeoIdentifierTag (e.g. compute a base offset using mx,my and add local
layerId), updating references in build(), buildStack(), makeEnv, and any code
that reads GeoIdentifierTag so it expects the encoded identifier; ensure layerId
incrementing still produces unique values and apply same fix to the other
occurrence around lines ~293-296.
🪄 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: 0173ed9e-1d2f-4c37-94a7-31da7b805cc1
📒 Files selected for processing (3)
src/SHiPMaterials.cppsubsystems/Calorimeter/CMakeLists.txtsubsystems/Calorimeter/src/CalorimeterFactory.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/SHiPMaterials.cpp
- subsystems/Calorimeter/CMakeLists.txt
Comment on lines
+185
to
+197
| int layerId = 0; // monotonic identifier used for GeoIdentifierTag | ||
|
|
||
| // Lambda: wrap a layer in a named air-box envelope inside the container | ||
| auto makeEnv = [&](const std::string& name, | ||
| double halfZ, | ||
| double zCenter) -> GeoPhysVol* | ||
| { | ||
| auto* s = new GeoBox(0.5*plateXY, 0.5*plateXY, halfZ); | ||
| auto* l = new GeoLogVol(name + "_env", s, airMat); | ||
| auto* p = new GeoPhysVol(l); | ||
| container->add(new GeoNameTag(name.c_str())); | ||
| container->add(new GeoIdentifierTag(layerId++)); | ||
| container->add(new GeoTransform(GeoTrf::Translate3D(offX, offY, zCenter))); |
There was a problem hiding this comment.
Make GeoIdentifierTags unique across modules.
build() places every module's layers directly under the same container, but layerId is reset to 0 on each buildStack() call. That means sibling volumes from different (mx, my) modules will reuse the same identifiers, so identifier-based lookup cannot distinguish modules reliably. Please either introduce a per-module parent volume or encode (mx, my, localLayer) into the tag before attaching children to container.
Also applies to: 293-296
🤖 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 185 - 197,
layerId is reset to 0 inside buildStack(), so GeoIdentifierTag instances created
by makeEnv are duplicated across modules (mx,my) when all layers are placed
under the same container; change the code so identifiers are globally unique per
module by either (A) making a per-module parent GeoPhysVol and attach layers to
that parent instead of the shared container, or (B) incorporate module
coordinates into the identifier before creating GeoIdentifierTag (e.g. compute a
base offset using mx,my and add local layerId), updating references in build(),
buildStack(), makeEnv, and any code that reads GeoIdentifierTag so it expects
the encoded identifier; ensure layerId incrementing still produces unique values
and apply same fix to the other occurrence around lines ~293-296.
Author
|
I don't understand the pre-commit ci error ? |
Contributor
Because you deleted the pre-commit config file (and a bunch of other files!) in your PR. Please revert these unrelated changes. I'll review the calorimeter changes tomorrow. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@subsystems/Calorimeter/src/CalorimeterFactory.cpp`:
- Around line 269-271: The ECAL branch for code == 8 updates zCursor but does
not increment gl, while the HCAL branch increments gl; make the behavior
explicit and consistent: either add "++gl" immediately after "zCursor +=
airGapZ;" in the ECAL branch to match HCAL, or (if air gaps intentionally count
as geometry layers only for HCAL) add a short clarifying comment in both
branches explaining the difference; update the code paths handling code == 8
(zCursor, airGapZ, gl) in CalorimeterFactory.cpp accordingly so the intent is
unambiguous.
- Around line 185-200: layerId is reset per buildStack(), causing duplicate
GeoIdentifierTag values across modules; modify the code so identifiers are
globally unique by either (A) making layerId a persistent counter (e.g. a member
or static counter used by buildStack()) or (B) encoding module indices into the
id when creating GeoIdentifierTag (e.g. compute id = mx*<scale1> + my*<scale2> +
layerId where <scale1>/<scale2> are chosen to avoid collisions); update the
makeEnv lambda to use the new id calculation or the shared counter when calling
container->add(new GeoIdentifierTag(...)) and keep GeoNameTag usage (mtag)
unchanged.
🪄 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: 4ed00c30-7d24-4015-87a0-de097a6e2244
📒 Files selected for processing (6)
src/SHiPGeometry.cppsrc/SHiPMaterials.cppsubsystems/Calorimeter/CMakeLists.txtsubsystems/Calorimeter/include/Calorimeter/CalorimeterFactory.hsubsystems/Calorimeter/src/CalorimeterFactory.cppsubsystems/Calorimeter/test_calorimeter.cpp
✅ Files skipped from review due to trivial changes (2)
- src/SHiPGeometry.cpp
- src/SHiPMaterials.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- subsystems/Calorimeter/CMakeLists.txt
Comment on lines
+269
to
+271
| } else if (code == 8) { | ||
| zCursor += airGapZ; | ||
|
|
There was a problem hiding this comment.
Inconsistent gl increment for air gap (code 8) between ECAL and HCAL.
In the ECAL section (line 270), code 8 only advances zCursor:
} else if (code == 8) {
zCursor += airGapZ;In the HCAL section (line 351), code 8 also increments gl:
} else if (code == 8) {
zCursor += airGapZ; ++gl;If this is intentional (air gaps count as "geometry layers" in HCAL but not in ECAL), a brief comment would clarify the design. Otherwise, consider aligning the behavior.
Also applies to: 350-352
🤖 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 269 - 271,
The ECAL branch for code == 8 updates zCursor but does not increment gl, while
the HCAL branch increments gl; make the behavior explicit and consistent: either
add "++gl" immediately after "zCursor += airGapZ;" in the ECAL branch to match
HCAL, or (if air gaps intentionally count as geometry layers only for HCAL) add
a short clarifying comment in both branches explaining the difference; update
the code paths handling code == 8 (zCursor, airGapZ, gl) in
CalorimeterFactory.cpp accordingly so the intent is unambiguous.
Contributor
These files all seem to be missing. |
Contributor
|
Could you please add the missing files? |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@subsystems/Calorimeter/src/CaloBarLayer.cpp`:
- Around line 18-27: In CaloBarLayer::place validate inputs up-front: check
tagPrefix is non-null before calling std::string(tagPrefix) (or accept empty
string), ensure barLog is non-null before constructing GeoPhysVol(barLog), and
ensure mother is non-null before calling mother->add(...); also validate nBars>0
and handle invalid values by throwing a descriptive std::invalid_argument or
logging an error and returning early. Include clear error messages naming the
failing parameter (e.g., "CaloBarLayer::place: null tagPrefix", "null barLog",
"null mother", "nBars <= 0") so callers can diagnose the cause.
In `@subsystems/Calorimeter/src/CaloFibreHPLayer.cpp`:
- Around line 39-55: The code in CaloFibreHPLayer attaches the casing to the
geometry tree (calls to mother->add with GeoNameTag/GeoTransform/casingPhys)
before validating fiberCoreDiam_mm (rCore), which can leave a partially built
geometry on error; move the validation of rCore (the check using
fiberCoreDiam_mm and rOuter) to the top of the function or at least before any
mother->add calls, or alternatively construct the casing and fibre volumes into
local temporary GeoPhysVol objects and only call mother->add(casingPhys) and
subsequent adds after the rCore validation succeeds so an exception cannot leave
a partially mutated geometry tree.
In `@subsystems/Calorimeter/src/CalorimeterConfig.cpp`:
- Around line 92-96: The "center_stack" branch currently lowercases val without
removing a trailing semicolon, so values like "true;" become "true;" and
cfg.center_stack is false; before transforming/normalizing val in the else if
(key == "center_stack") block, strip any trailing semicolon (same normalization
used for the numeric keys), then lowercase and set cfg.center_stack based on the
cleaned string (compare to "1", "true", "yes", "on") so cfg.center_stack is
parsed correctly; locate the branch handling key == "center_stack" and apply the
same trailing-semicolon trimming used elsewhere in CalorimeterConfig.cpp.
🪄 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: 2612f7e8-68b7-4cb6-800c-c0d880f9bc42
📒 Files selected for processing (6)
subsystems/Calorimeter/include/Calorimeter/CaloBarLayer.hsubsystems/Calorimeter/include/Calorimeter/CaloFibreHPLayer.hsubsystems/Calorimeter/include/Calorimeter/CalorimeterConfig.hsubsystems/Calorimeter/src/CaloBarLayer.cppsubsystems/Calorimeter/src/CaloFibreHPLayer.cppsubsystems/Calorimeter/src/CalorimeterConfig.cpp
Comment on lines
+18
to
+27
| void CaloBarLayer::place(GeoVPhysVol* mother, | ||
| GeoLogVol* barLog, | ||
| double pitch_mm, | ||
| int nBars, | ||
| double zCenter_mm, | ||
| const char* tagPrefix, | ||
| int layerIndex, | ||
| BarAxis axis, | ||
| const std::string& nameSuffix) | ||
| { |
There was a problem hiding this comment.
Add defensive validation for API inputs before placement.
Line 39 (std::string(tagPrefix)) and Lines 44–47 (GeoPhysVol(barLog), mother->add(...)) assume all pointers are valid. A null tagPrefix, barLog, or mother will hard-fail at runtime; nBars <= 0 silently builds nothing.
Proposed fix
`#include` "Calorimeter/CaloBarLayer.h"
@@
`#include` <string>
+#include <stdexcept>
@@
void CaloBarLayer::place(GeoVPhysVol* mother,
GeoLogVol* barLog,
double pitch_mm,
int nBars,
@@
BarAxis axis,
const std::string& nameSuffix)
{
+ if (mother == nullptr) throw std::invalid_argument("CaloBarLayer::place: mother is null");
+ if (barLog == nullptr) throw std::invalid_argument("CaloBarLayer::place: barLog is null");
+ if (tagPrefix == nullptr) throw std::invalid_argument("CaloBarLayer::place: tagPrefix is null");
+ if (nBars <= 0) throw std::invalid_argument("CaloBarLayer::place: nBars must be > 0");
+
const double pitch = pitch_mm * mm;
const double s0 = -0.5 * (nBars - 1) * pitch;Also applies to: 38-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@subsystems/Calorimeter/src/CaloBarLayer.cpp` around lines 18 - 27, In
CaloBarLayer::place validate inputs up-front: check tagPrefix is non-null before
calling std::string(tagPrefix) (or accept empty string), ensure barLog is
non-null before constructing GeoPhysVol(barLog), and ensure mother is non-null
before calling mother->add(...); also validate nBars>0 and handle invalid values
by throwing a descriptive std::invalid_argument or logging an error and
returning early. Include clear error messages naming the failing parameter
(e.g., "CaloBarLayer::place: null tagPrefix", "null barLog", "null mother",
"nBars <= 0") so callers can diagnose the cause.
Author
|
I'm not too sure what is wrong with the ci? It says something about gersemi not finding the rust compiler? |
Contributor
That's unrelated, fixed already in FairShip, but forgot to also apply the fix here. |
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
subsystems/Calorimeter/src/CaloFibreHPLayer.cpp (1)
31-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidation occurs after geometry mutation.
The casing is attached to
motherat Lines 36-38 before the fibre diameter validation at Lines 44-47. IfrCore <= 0orrCore > rOuter, the function throws but leaves a partially built HPL (the casing) in the geometry tree. Move the validation to before the firstmother->add(...)call.Proposed fix
void CaloFibreHPLayer::build(GeoVPhysVol* mother, GeoMaterial* aluminiumMat, GeoMaterial* fibreMat, const std::string& layerTag, double zCenter_mm, int layerIndex, double casingXY_mm, double casingZ_mm, double fiberDiam_mm, double fiberCoreDiam_mm, bool fibresAlongY, const std::string& nameSuffix) { const double casingXY = casingXY_mm * mm; const double casingZ = casingZ_mm * mm; + // ── validate fibre geometry before mutating the tree ────────────────── + const double rOuter = 0.5 * fiberDiam_mm * mm; + const double rCore = 0.5 * fiberCoreDiam_mm * mm; + + if (rCore <= 0.0 || rCore > rOuter) + throw std::runtime_error( + "CaloFibreHPLayer: invalid core diameter " + "(must be >0 and <= outer diameter)"); + // ── aluminium casing ────────────────────────────────────────────────── auto* casingShape = new GeoBox(0.5 * casingXY, 0.5 * casingXY, 0.5 * casingZ); auto* casingLog = new GeoLogVol("HPL_CasingLog", casingShape, aluminiumMat); auto* casingPhys = new GeoPhysVol(casingLog); mother->add(new GeoNameTag((layerTag + "_HPL_Casing" + nameSuffix).c_str())); mother->add(new GeoTransform(GeoTrf::Translate3D(0.0, 0.0, zCenter_mm * mm))); mother->add(casingPhys); - // ── fibre geometry ──────────────────────────────────────────────────── - const double rOuter = 0.5 * fiberDiam_mm * mm; - const double rCore = 0.5 * fiberCoreDiam_mm * mm; - - if (rCore <= 0.0 || rCore > rOuter) - throw std::runtime_error( - "CaloFibreHPLayer: invalid core diameter " - "(must be >0 and <= outer diameter)"); - // GeoTube axis is Z; rotate so fibres run along Y or X🤖 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/Calorimeter/src/CaloFibreHPLayer.cpp` around lines 31 - 47, The validation of fibre radii (rCore and rOuter) is happening after the casing geometry (casingShape/casingLog/casingPhys) is already attached to the geometry tree via mother->add; move the check for rCore <= 0 or rCore > rOuter to before any mutation of mother (i.e., before creating/adding casingShape/casingLog/casingPhys and before the first mother->add call) so that the function throws without leaving a partially-built GeoPhysVol in the tree; ensure the variables rOuter and rCore are computed prior to that validation and only proceed to construct and add the GeoBox/GeoLogVol/GeoPhysVol and GeoNameTag/GeoTransform when the validation passes.
🤖 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/Calorimeter/test_calorimeter.cpp`:
- Line 4: The test build fails because test_calorimeter.cpp includes
"Calorimeter/CalorimeterConfig.h" but the header is not visible to the test
target; fix by wiring the Calorimeter library as a dependency or exporting its
include directories to the test target: ensure the test CMake target calls
target_link_libraries(<test target> PRIVATE|PUBLIC Calorimeter) or
target_include_directories(<test target> PRIVATE
$<TARGET_PROPERTY:Calorimeter,INTERFACE_INCLUDE_DIRECTORIES>) so that
CalorimeterConfig.h (from the Calorimeter target) is available; alternatively,
if the header is supposed to be local to the test, change the include to a
relative path to the header, but prefer linking the Calorimeter target so
symbols like CalorimeterConfig.h resolve consistently in test builds.
---
Duplicate comments:
In `@subsystems/Calorimeter/src/CaloFibreHPLayer.cpp`:
- Around line 31-47: The validation of fibre radii (rCore and rOuter) is
happening after the casing geometry (casingShape/casingLog/casingPhys) is
already attached to the geometry tree via mother->add; move the check for rCore
<= 0 or rCore > rOuter to before any mutation of mother (i.e., before
creating/adding casingShape/casingLog/casingPhys and before the first
mother->add call) so that the function throws without leaving a partially-built
GeoPhysVol in the tree; ensure the variables rOuter and rCore are computed prior
to that validation and only proceed to construct and add the
GeoBox/GeoLogVol/GeoPhysVol and GeoNameTag/GeoTransform when the validation
passes.
🪄 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: 30df109d-6c5d-4fcf-9726-2f8c29b95dc2
📒 Files selected for processing (12)
src/SHiPGeometry.cppsrc/SHiPMaterials.cppsubsystems/Calorimeter/CMakeLists.txtsubsystems/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.cpp
✅ Files skipped from review due to trivial changes (1)
- src/SHiPMaterials.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/SHiPGeometry.cpp
- subsystems/Calorimeter/CMakeLists.txt
| // SPDX-License-Identifier: LGPL-3.0-or-later | ||
| // Copyright (C) CERN for the benefit of the SHiP Collaboration | ||
|
|
||
| #include "Calorimeter/CalorimeterConfig.h" |
There was a problem hiding this comment.
Build blocker: CalorimeterConfig.h is not resolvable in this test target.
Static analysis reports a hard compile error here ('Calorimeter/CalorimeterConfig.h' file not found). Please fix the target include/export wiring (preferred) or adjust the include path so this header resolves consistently in test builds.
🧰 Tools
🪛 Clang (14.0.6)
[error] 4-4: 'Calorimeter/CalorimeterConfig.h' file not found
(clang-diagnostic-error)
🤖 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/Calorimeter/test_calorimeter.cpp` at line 4, The test build fails
because test_calorimeter.cpp includes "Calorimeter/CalorimeterConfig.h" but the
header is not visible to the test target; fix by wiring the Calorimeter library
as a dependency or exporting its include directories to the test target: ensure
the test CMake target calls target_link_libraries(<test target> PRIVATE|PUBLIC
Calorimeter) or target_include_directories(<test target> PRIVATE
$<TARGET_PROPERTY:Calorimeter,INTERFACE_INCLUDE_DIRECTORIES>) so that
CalorimeterConfig.h (from the Calorimeter target) is available; alternatively,
if the header is supposed to be local to the test, change the include to a
relative path to the header, but prefer linking the Calorimeter target so
symbols like CalorimeterConfig.h resolve consistently in test builds.
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.
Please check that it works as expected, tests are passed but I am not sure what they entail
Summary by CodeRabbit
New Features
Tests
Chores